Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a debuger function to check wrapper content #1461

Closed
SergiCL opened this issue Mar 9, 2020 · 14 comments · Fixed by #1491, eddieferrer/sizesquirrel-open#212 or nextcloud/social#1222
Closed

Comments

@SergiCL
Copy link
Contributor

SergiCL commented Mar 9, 2020

What problem does this feature solve?

Actually, if you try to see the contents of your wrapper the result is a little bit confusing.
It could work when wrappers from simple elements, but if biger it tursn unreadable:

console.log(simpleWrapper)
// output:  Wrapper { selector: 'p' }

console.log(bigerWrapper)
// output:  VueWrapper { isFunctionalComponent: undefined, emitted: [Object: null prototype] {},_emittedByOrder: [] }

One posible alternative could be to use .html() before log the content, but this function doesen't work with array wrappers.

My proposal is to add a function to the wrappers that allows to visualize its content in a clear and simple way, without worrying about what kind of wrapper it is. A well formated pretty print of the html of the wrapper element, who gives support to array wrappers too.

It can be very useful to understand what is really being tested, especially for people in their first steps who are still learning how the library works.

What does the proposed API look like?

For simple wrapper:

wrapper.debug()

// Output:
 Wrapper:
    <div>
      <p>1</p>
    </div> 

For array wrapper:

wrapperArray.debug()

// Output:
Wrapper Array:
   At 0:
    <div>
      <p>1</p>
    </div> 

  At 1:
    <div>
      <p>2</p>
    </div> 

  At 2:
    <div>
      <p>3</p>
    </div> 
@afontcu
Copy link
Member

afontcu commented Mar 9, 2020

Hi! This is an interesting suggestion, we're currently focused on releasing v1.0 but we'll keep it in mind for future releases :)

By the way, you could log your wrapped elements in a (arguably more) convoluted way:

wrapper.findAll('p').wrappers.forEach(wrapper => console.log(wrapper.html()))

or even

console.log([...wrapper.findAll('p').wrappers.map(wrapper => wrapper.html())])

it is not nice, but we'll do the trick 😇

@SergiCL
Copy link
Contributor Author

SergiCL commented Mar 9, 2020

it is not nice, but we'll do the trick 😇

Is a bit tricky and hard to read, but it's for debugging so it's right for me.

we're currently focused on releasing v1.0 but we'll keep it in mind for future releases :)

Given that you're busy and since it doesn't seem like a very complex functionality. Is it okay if I try to implement it, or it's better to wait until v1.0 is released?

@JessicaSachs
Copy link
Collaborator

JessicaSachs commented Mar 10, 2020 via email

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 10, 2020

What exactly is the spec for debug going to be? Just pretty printing the HTML?

If that is the case, rather than add a new method, could we just do something like wrapper.html({ pretty: true })?

For wrapper array you can just do .map(x => x.html({ pretty: true })). You could use this: https://www.npmjs.com/package/pretty although it's not perfect, it's still pretty good.

SergiCL pushed a commit to SergiCL/vue-test-utils that referenced this issue Mar 14, 2020
@SergiCL SergiCL mentioned this issue Mar 14, 2020
6 tasks
@SergiCL
Copy link
Contributor Author

SergiCL commented Mar 14, 2020

What exactly is the spec for debug going to be? Just pretty printing the HTML?

You're right, at this point the debugging function doesn't bring much more than a pretty. But it helps to encapsulate this functionality and improve it in next phases, adding more useful information.

@lmiller1990
Copy link
Member

If we are going to add additional debugging (I think this might be hard, depending what you want to support) I think we should probably decide exactly what we want to add before we do so. If you still think this is a essential feature, can you do a proposal for what we should (and should not) add?

My personal feeling is that if people want to debug, they should use a real debugger, which will do the job much better. Any reason not to just do that?

@SergiCL
Copy link
Contributor Author

SergiCL commented Mar 15, 2020

If you still think this is a essential feature, can you do a proposal for what we should (and should not) add?

Maybe is not essential, probably is more a nice-to-have, but I think it's simple and could add a lot of value.

It's possible that to add additional debugging may be complicated and unnecesary and as you said, you should use a debugger. I was talking about more simple details (e.g. array length)

In any case 'debug' can bring advantages over a pretty print. Some of them could be:

  • Highlighted text
  • Set max length of printed output
  • Save output to a file to make it more readable
  • Set a lintern to warn if debug function is used, to prevent it from going into production

All of them could be added as function parameters in future steps.

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 15, 2020

My thoughts:

  • highlighted text: Probably better to leave this to a test runner, would be interested in seeing what kind of improvements you are thinking of, though. Can you share your ideas (no rush, take your time)?
  • max length: personally I think it's better just to dump everything. Can you share an example where setting a max length is beneficial?
  • output file: I think most test runners have something like this already. A file-by-file report on coverage, failures and the speed of each test would be something I'm very interested in though. I don't know how we would implement this in VTU. Again, I think this might be something you do as a test runner level plugin.
  • Linter rule: A linter can already catch this with a custom rule. Also, tests never go to production (?)

It sounds like we are more or less reinventing what tests runners already do here, but I am open to changing my mind with some examples. One thing I would really like to see if printing the HTML output of a failed test based on a global configuration variable, like your original proposal. Something like

import { config } from '@vue/test-utils'
config.debugLevel = 1 (we could have 0, 1, 2, each getting more verbose)

0 would be what we have now. 1 could automatically print the HTML and variables on vm.data or something.

@SergiCL
Copy link
Contributor Author

SergiCL commented Mar 16, 2020

output file: I think most test runners have something like this already. A file-by-file report on coverage, failures and the speed of each test would be something I'm very interested in though. I don't know how we would implement this in VTU. Again, I think this might be something you do as a test runner level plugin.

In the end what we would be doing is printing formatted html code (and maybe additional information). Sometimes this code can be too long to read comfortably in the console, so it would be nice to save this code in a file to read it comfortably.
This could be done passing a param (save? or fileName?) to enable this function.

max length: personally I think it's better just to dump everything. Can you share an example where setting a max length is beneficial?

MaxLength is a patch for the same problem. Instead of print all the code, we can print some of it to get an idea of what we're dealing with.
Although well thought out having several levels of verbose can be more useful.

Linter rule: A linter can already catch this with a custom rule. Also, tests never go to production (?)

Bad choice of words 😂.
The 'debug' function is an aid during development. I meant that it should not stay in the final tests, and this could be controlled with a custom rule.

0 would be what we have now. 1 could automatically print the HTML and variables on vm.data or something.

vm.data is a perfect example of useful information that could be added in addition to the pretty print

On the other hand I am not totally against a solution like wrapper.html({ pretty: true }) but I think its a little messy for wrapper array.

I guess calling it debug can be confusing, maybe something like print or log could be better? Any other suggestions?

When I have time I will add some examples of a possible, more definitive, implementation.

@lmiller1990
Copy link
Member

I look forward to your design spec - I'd recommend we figure out the final API design before coding too much - it would be a shame to work on something then not get it merged due to complexity or some other issue. I'm personally still not sold on this - I think these are mostly solved problems by a regular debugging tool and a test runner in the majority of cases, but still happy to see what comes of this discussion.

@SergiCL
Copy link
Contributor Author

SergiCL commented Mar 21, 2020

Hi everyone, I've been thinking about this for the last few days trying to get a better defininition of this hypotetical function.

I will try to give a clearer definition both of the purpose of the function and of those cases in which it might be useful. I would also like to give some visual examples of a possible output and the parameters that the function could use.

Objective:
"To give an overview of the state of the wrapper at the time the function is executed"

(Maybe wrapper.overview() is a better name)

When it could be useful
Any time you need to know what's going on with the wrapper.

I can think of two key cases:

  • To learn/teach: If someone has just started testing their component, having a simplified view of what the wrapper contains can be very enlightening.

  • To detect the error: I am aware that that is what debuggers are for (which will surely do it better) but it isn't intended to be an alternative, but rather a support. Having an overview can be a good first step to know where to look.
    With this in mind it would be especially interesting to launch this function automatically if a test fails, making debugging easier.

Posible output
The overview should include only useful information for the tester, in a simple and direct way.

After some thought, I think the most useful data would be

  • Wrapper visibility
  • Html
  • Data state
  • Emitts' information

Other possible elements to add could be computed properties or styles, but I think that would increase the size of the overview too much.
I don't see the need to add Class block due to they are easy to see in the HTML, but they could also be added if deemed necessary.

A posible output with for this information could be something like this:

wrapper.overview()

// output
Wrapper (Visible):

Html:
  <div class="test">
    <p>My name is Tess Ting</p>
  </div>

Data: {
  "firstName":	"Tess",
  "lastName":	"Ting"
}

Emitted: {
  "foo": [
    1.	["hello",	"world"],
    2.	["bye", 	"world"]
}

The information is organized and that makes it easier to locate the main points that our test would attack.
In this way you can easily see why the test is failing, or have a clearer view of what is being tested.

Create your own overview
This is propapbly a second or third step, but Í'm going to explain the general idea.
Having the overview separated into blocks, it is easy to choose what is or is not useful for you and to customize it as desired.
This customization could be carried out in the configuration (for all the proyect) or using params (only for that execution).

Posible example

// We will considere this default configuration
// config.overview = {
//	'visibility': true
//	'html': true,
//	'data': true,
//	'emitted': true
//}

wrapper.overview({visibility: false, html: false, emitted: false})

// Output
Wrapper:

Data: {
  "firstName":	"Tess",
  "lastName":	"Ting"
}

I hope this helps to clarify a little the reason for this issue.
I'm aware that there will be more important tasks than this at this moment, at the end isn't more than a nice-to-have, so thank you for your time.

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 23, 2020

Thoughts:

  • html, data, emitted and computed make sense to me. visible is hard (since if it's got display: none from a css rule, we have no way to know that.
  • I don't think step 2/3 is really necessary, why not just show everything?
  • I like the name overview much better than debug.
  • You should be able to build this entirely with the existing VTU APIs, which should mean little to no increased maintenance if we add this feature.
  • I think we should be careful to avoid feature scope: eg, I feel like the next step is someone will ask to show data, computed etc for all nested components etc - if we can keep this simple, I think it's okay to add it. Is the above what you think will be the final spec?

@SergiCL
Copy link
Contributor Author

SergiCL commented Mar 23, 2020

html, data, emitted and computed make sense to me. visible is hard (since if it's got display: none from a css rule, we have no way to know that.

I thought about using the 'visible' function of the wrapper itself. Wouldn't that work?

I don't think step 2/3 is really necessary, why not just show everything?

Yeah, it's probably unnecessary, showing everything should be fine.

I like the name overview much better than debug.

Great, it's a clearer name, better to avoid confusion.

You should be able to build this entirely with the existing VTU APIs, which should mean little to no increased maintenance if we add this feature.

It's something I was thinking about while I was defining the specification. The less maintenance it needs, the better.

I think we should be careful to avoid feature scope: eg, I feel like the next step is someone will ask to show data, computed etc for all nested components etc - if we can keep this simple, I think it's okay to add it. Is the above what you think will be the final spec?

For me this should be a final version. Something more complex is not an overview.

Well, if it's okay with you, I'll get to work on it when I get a chance. I should keep the PR open and update it or close it and open a new one to avoid confusion?

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 23, 2020

You can use visible if you want; be aware it has the same limitation as explained above.

Go for it; you can open a new PR or use the existing one, up to you. I don't think this adds much maintenance overhead since it's basically built using the existing public API.

@SergiCL SergiCL closed this as completed Mar 27, 2020
@SergiCL SergiCL reopened this Mar 27, 2020
SergiCL pushed a commit to SergiCL/vue-test-utils that referenced this issue Mar 28, 2020
lmiller1990 pushed a commit that referenced this issue Apr 8, 2020
* feat(test-utils): add 'overview' function

close #1461

* feat(test-utils): add error wrapper overview

* fix(test-utils): fix flow errors

* fix(test-utils): compatibility with older versions

* fix(test utils): flow error

* feat: overview function PR comments

Co-authored-by: root <root@LAPTOP-VOQ3N65A.localdomain>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment