search

avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

Limit the number of statements in the output for t.true/t.false #1204

Closed vadimdemedes closed 4 years ago

vadimdemedes commented 7 years ago

Note, this issue is related only to #1154.

Given this assertion:

const obj = {
  a: {
    b: {
      c: [false]
    }
  }
};

t.true(obj.a.b.c[0]);

AVA will output the value of each statement (obj.a.b.c[0], obj.a.b.c, obj.a.b, obj.a, obj), which of course is not optimal, since the helpful values are probably only the last two. We should limit the number of statements to prevent a long list of unhelpful values.

novemberborn commented 7 years ago

https://github.com/avajs/ava/issues/1222#issuecomment-280671306 is another example of this. I can reproduce that using:

import test from 'ava'
import {jsdom} from 'jsdom'

test(t => {
  const doc = jsdom('<html></html>')
  t.true(doc.documentElement.classList.contains('foo'))
})

With AVA 0.18.1 we dump out each property in that MemberExpression. With AVA 0.17.0 we only show:

  t.true(doc.documentElement.classList.contains('foo'))
                                       |
                                       false

Why did we remove the old formatter code? To get consistency in how we were displaying objects etc?

Would it be an appropriate short-term fix to just show the last property, and hope it's not too massive?

Longer term it might still be useful to show each property in the MemberExpression, but perhaps just their type? I don't think we can do that with pretty-format though.

vadimdemedes commented 7 years ago

Why did we remove the old formatter code? To get consistency in how we were displaying objects etc?

Yeah, for consistency. Otherwise it'd be pretty weird to see completely different looking output for different assertions.

Would it be an appropriate short-term fix to just show the last property, and hope it's not too massive?

Totally agree. Perhaps we could even remove "statements" output for t.true/t.false until it's more production ready and just show actual/expected. What do you think?

Longer term it might still be useful to show each property in the MemberExpression, but perhaps just their type? I don't think we can do that with pretty-format though.

Output would still be pretty long, not sure we'd want to show each property. I'd go for a proposal in #1205.

novemberborn commented 7 years ago

Alternatively we could limit the depth to 1.

vadimdemedes commented 7 years ago

Also a possibility, but I'm pretty sure it wouldn't solve the problem with the long output.

sindresorhus commented 7 years ago

Perhaps we could even remove "statements" output for t.true/t.false until it's more production ready and just show actual/expected.

👍 I think we should do that for now and work on improving the output before turning it on again.

novemberborn commented 7 years ago

That doesn't leave us with much power-assert output does it?

vadimdemedes commented 7 years ago

That doesn't leave us with much power-assert output does it?

Yes, since "statements" output is the only thing using it.

novemberborn commented 7 years ago

Some more examples from @davewasmer (https://github.com/avajs/ava/issues/1278):

I have several tests that assert that a file exists:

t.true(fs.existsSync(someFilePath), 'file exists');

In this case, magic-assert ends up printing the entirety of the fs object, which is ~170 lines long, and not super helpful here. In another case, I assert a value on t.context:

t.true(fs.existsSync(t.context.someFilePath), 'file exists');

magic-assert dumps t here, which ends up being nearly 800 lines long. This means lots of scrolling to find the info I actually care about (the value of t.context.someFilePath).

We should definitely filter out Node's built-in module, and the t object.

davewasmer commented 7 years ago

Just to weigh in here from #1278 - it seems like there's going to be suboptimal tradeoffs no matter what you do here. If you limit it to the last two members of a MemberExpression, that would still dump the entire fs module for fs.existsSync(). You could blacklist Node core libs and t, but that starts to feel like whack-a-mole: what if I use fs-extra or mock-fs? You could only show the last member (i.e. filepath on t.context.filepath), but perhaps t.context would be helpful to dump there?

I'll echo my suggestion in #1278 here - if the magic-assert info could be exposed in a machine readable way, that opens up the door to more userland experimentation with different reporter styles. But based on other issue threads, it sounds like userland reporters is something you intentional don't want to support - if that's the case, then feel free to disregard these comments :wink:.

For a bit of background / context: my motivation here is that Denali uses ava for running tests. If ava exposed more machine readable output, Denali could do some really interesting / cool things with that output since it knows much more about app than ava ever could.

novemberborn commented 7 years ago

If ava exposed more machine readable output, Denali could do some really interesting / cool things with that output since it knows much more about app than ava ever could.

Despite my comment in https://github.com/avajs/ava/issues/1278#issuecomment-281412664, really interesting features can always sway us :)

novemberborn commented 7 years ago

This now needs https://github.com/concordancejs/concordance/issues/12 to land.

hoschi commented 7 years ago

Does "properties" also mean return values from function calls? E.g. I have this line in my test: t.truthy(header.find(TitleMedium).prop('title') === props.title); which should not log header.find(TitleMedium)

I don't know how much properties you want to log for each object in a test line, but probably it makes sense to count all the lines you logged already so you can check if this exceeds a limit. At the moment the statement above logs around 5 pages of screen space :grinning:

novemberborn commented 7 years ago

Does "properties" also mean return values from function calls?

@hoschi, it does, yes.

I don't know how much properties you want to log for each object in a test line, but probably it makes sense to count all the lines you logged already so you can check if this exceeds a limit.

The idea is to log a few properties for each object. It's pretty hard to find the correct trade-off though.

hoschi commented 7 years ago

It's pretty hard to find the correct trade-off though. correct, because you not know how much lines a nested object produce when you log "3 props from each, down 4 levels". I thought that is the main problem with this approach? So I suggested to do this not with such a static approach, but in a recursive like way to have more control on the output:

t.truthy(header.name === config.title) here header and config are objects which say have a lot of properties which are nested objects too, only title and name are strings. Then this could be an example of the idea:

With this you would make sure that "small" props (bool, date, string, ...) are rendered with an higher priority. With checking the rendered lines before recursing one level deeper, you know if you have already rendered a screen page of information and can stop here. This can be configurable to match font configs in IDEs/terminal.

I hope it makes more sense now :grinning:

novemberborn commented 7 years ago

@hoschi yes thank you, that's a nice way of thinking about it. It's horribly complicated though! 😄