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

vadimdemedes · 2017-01-23T17:54:33Z

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.

The text was updated successfully, but these errors were encountered:

novemberborn · 2017-02-17T16:41:10Z

#1222 (comment) 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 · 2017-02-17T22:10:32Z

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 · 2017-02-18T14:15:59Z

Alternatively we could limit the depth to 1.

vadimdemedes · 2017-02-18T18:26:36Z

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

sindresorhus · 2017-02-20T07:35:32Z

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 · 2017-02-20T10:13:06Z

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

vadimdemedes · 2017-02-20T13:28:31Z

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

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

novemberborn · 2017-02-21T10:41:47Z

Some more examples from @davewasmer (#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 · 2017-02-21T16:49:13Z

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 😉.

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 · 2017-02-21T17:21:28Z

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 #1278 (comment), really interesting features can always sway us :)

novemberborn · 2017-06-28T13:31:56Z

This now needs concordancejs/concordance#12 to land.

hoschi · 2017-09-19T09:23:55Z

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 😀

novemberborn · 2017-09-24T14:20:03Z

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 · 2017-09-25T10:01:59Z

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:

render name and title (2 lines)
process first level of props of header and config, there is still a config for this e.g. process only 3 props
- if prop is object or array, put it on stack
- if prop is string, number, bool, date (one liners) render them
check if line count exceeds limit
process item from stack
recursive into properties which are of type array or object

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 😀

novemberborn · 2017-10-01T15:12:18Z

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

vadimdemedes added enhancement new functionality future we'll get back to this eventually labels Jan 23, 2017

vadimdemedes assigned vadimdemedes and unassigned vadimdemedes Jan 23, 2017

This was referenced Feb 17, 2017

Show relevant parts of the object/array for t.true/t.false assertion output #1205

Closed

t.is not working, however is more strict cousin t.deepEqual is working #1222

Closed

novemberborn added bug current functionality does not work as desired and removed future we'll get back to this eventually labels Feb 17, 2017

novemberborn mentioned this issue Feb 21, 2017

Magic assert output is very noisy #1278

Closed

vadimdemedes added the magic-assert label Feb 25, 2017

novemberborn mentioned this issue Mar 15, 2017

More assertion changes #1312

Merged

novemberborn mentioned this issue Aug 5, 2017

Proposal, support for t.all() #1482

Closed

novemberborn mentioned this issue Sep 3, 2017

Include only plain objects/arrays in t.true/t.false output of statements #1226

Closed

novemberborn added scope:assertions and removed scope:magic-assert labels Jan 19, 2020

novemberborn closed this as completed May 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vadimdemedes commented Jan 23, 2017

novemberborn commented Feb 17, 2017

vadimdemedes commented Feb 17, 2017

novemberborn commented Feb 18, 2017

vadimdemedes commented Feb 18, 2017

sindresorhus commented Feb 20, 2017 •

edited

Loading

novemberborn commented Feb 20, 2017

vadimdemedes commented Feb 20, 2017

novemberborn commented Feb 21, 2017

davewasmer commented Feb 21, 2017 •

edited by novemberborn

Loading

novemberborn commented Feb 21, 2017

novemberborn commented Jun 28, 2017

hoschi commented Sep 19, 2017

novemberborn commented Sep 24, 2017

hoschi commented Sep 25, 2017

novemberborn commented Oct 1, 2017

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

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

Comments

vadimdemedes commented Jan 23, 2017

novemberborn commented Feb 17, 2017

vadimdemedes commented Feb 17, 2017

novemberborn commented Feb 18, 2017

vadimdemedes commented Feb 18, 2017

sindresorhus commented Feb 20, 2017 • edited Loading

novemberborn commented Feb 20, 2017

vadimdemedes commented Feb 20, 2017

novemberborn commented Feb 21, 2017

davewasmer commented Feb 21, 2017 • edited by novemberborn Loading

novemberborn commented Feb 21, 2017

novemberborn commented Jun 28, 2017

hoschi commented Sep 19, 2017

novemberborn commented Sep 24, 2017

hoschi commented Sep 25, 2017

novemberborn commented Oct 1, 2017

sindresorhus commented Feb 20, 2017 •

edited

Loading

davewasmer commented Feb 21, 2017 •

edited by novemberborn

Loading