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

Snapshot report descriptions #3071

Open
panoply opened this issue Jul 23, 2022 · 7 comments
Open

Snapshot report descriptions #3071

panoply opened this issue Jul 23, 2022 · 7 comments

Comments

@panoply
Copy link

panoply commented Jul 23, 2022

Description

Firstly, thank you for creating and maintaining this brilliant tool. I leverage AVA's snapshot feature for comparing responses generated from parsers/lexers and it would be nice if I could feed descriptions of the snapshot test into reports to help better understand what is going in and what I am testing etc.

Currently and AFAIK ava provides this functionality as "labels" when passing a string to the 2nd parameter in the .snapshot() method which will write to the quoted > output, eg:

test('Some example test name', t => {

  const source = '<div>Hello World</div>';

  t.snapshot(source, 'Testing hello world');

})

The above example will output the following in a markdown file:

# Snapshot report for `tests/eg.test.mjs`

The actual snapshot is saved in `eg.mjs.snap`.

Generated by [AVA](https://avajs.dev).

## Some example test name

> Testing hello world

    '<div>Hello World</div>'

While this suffices for a lot of cases, it does not allow for me (and maybe others) to have clear understanding on the purpose of the test and it becomes exceptionally difficult when reviewing snap reports. The current label and test title (even when multi-line) are not enough. Below is an example to better visualize what I mean.

Notice the generate snapshot reports include a description following the test title:

# Snapshot report for `tests/eg.test.mjs`

The actual snapshot is saved in `eg.mjs.snap`.

Generated by [AVA](https://avajs.dev).

## Some example test name

An example of where the description should be output.

> Testing hello world (snapshot 1)

    '<div>Hello World</div>'

> Testing something else (snapshot 2)

    '<div>etc etc etc</div>'

> Testing some else (snapshot 3)

    'xxx xxx xxx xxx'

In the above, a description of the test is applied to the report. Passing markdown descriptions would be ideal but simple multiline strings would also suffice too.

Why you can't use AVA for this

There is no way to apply this (AFAIK) in reports.

And maybe how you think AVA could handle this

Personally, I haven't looked at the overall implementation approach one might employ to support such a capability and I am unsure of how ava is handling this aspect internally. Ideally and given the feature request pertains to snapshot assertions, exposing a description or describe to the t.snapshot method seems elegant and non-intrusive (ie: t.snapshot.describe()). IIRC I once read an issue where someone proposed something similar to be applied on the label parameter but that feels a tad extraneous.

To better demonstrate, see the below example:

_In this I am assuming the following type: t.snapshot.describe(...description: string[]) and also passing some markdown, grain of salt that aspect.

test('Some example test name', t => {

 // ...
  
  t.snapshot.describe(
    'The snapshot report description can be provided here.',
    'The `describe` method similar to `t.log` could accept spread argument and',
    'maybe some low level **markdown** could be accepted if it does not interfere',
    'with the existing logic, for example:',
    '- List item 1',
    '- List item 2'
    'etc etc',
  )
 
  t.snapshot(source, 'The snapshot label');
  
})

The resulting report would be:

# Snapshot report for `tests/eg.test.mjs`

The actual snapshot is saved in `eg.mjs.snap`.

Generated by [AVA](https://avajs.dev).

## Some example test name

The snapshot report description can be provided here. The `describe` method similar to `t.log` could accept spread argument and maybe some low level **markdown** could be accepted if it does not interfere with the existing logic, for example:

- List item 1
- List item 2

etc etc

> The snapshot label

    'xxx xxx xxx xxx'
@novemberborn
Copy link
Member

Interesting!

Could perhaps do this as a t.describe() which we could then also include in log output in case the test fails. We'd allow only one call per test, and could perhaps require it to occur before snapshot assertions.

We'd need to add it to the binary snapshot encoding so it's preserved when the Markdown file is re-created when a new test is added.

What do you think?

@panoply
Copy link
Author

panoply commented Jul 24, 2022

Could perhaps do this as a t.describe() which we could then also include in log output in case the test fails. We'd allow only one call per test, and could perhaps require it to occur before snapshot assertions.

Sounds utterly delightful. I had a brief look at the existing logic and it seems applicable to employ with not too much refactors or adjustments, though I might be wrong. Extending the log output to support descriptives is also a fantastic idea, but may require some additional thought if markdown is passed, right?

Originally, I wanted to be able to apply descriptions for every snapshot assertion in a test. In my use cases I have multiple snapshot assertions, eg:

test('xxx', t => {

  t.snapshot(source1, 'some label');
  
  t.snapshot(source2, 'another label');
  
  t.snapshot(source2, 'yet another label');
})

Being able to inform or better describe each snapshot would be the crème de la crème - meaning markdown reports would be able to generate something like:

# Snapshot report for `tests/eg.test.mjs`

The actual snapshot is saved in `eg.mjs.snap`.

Generated by [AVA](https://avajs.dev).

## Some example test name

An example of where the description should be output.

### some label

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

> Testing hello world (snapshot 1)

    '<div>Hello World</div>'

### another label

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

> Testing something else (snapshot 2)

    '<div>etc etc etc</div>'

### yet another label

Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

> Testing some else (snapshot 3)

    'xxx xxx xxx xxx'

However, something of this nature feels a little too much, so I would definitely settle for anything which provides descriptive capabilities in a smart and logical manner (as per your suggestion).

Thanks for the speedy reply and hearing out the suggestion.

@novemberborn
Copy link
Member

Extending the log output to support descriptives is also a fantastic idea, but may require some additional thought if markdown is passed, right?

I'd imagine we render it as text, without additional parsing.

Being able to inform or better describe each snapshot would be the crème de la crème

Perhaps the current display style of blockquotes is hampering this? We could do something like:

## Test title

### Snapshot 1

Here's the snapshot message text, it can be Markdown if you like.

    'snapshot'

@panoply
Copy link
Author

panoply commented Jul 28, 2022

Also good. For some context, here is an example of snapshot labels I manually augment before assertion. Might give you some idea of the practicality for the descriptives that one can employ.

Perhaps the current display style of blockquotes is hampering this?
Yes, indeed.

@novemberborn
Copy link
Member

@panoply where do you think we should start? If we change the display style is that enough, or would you also need the t.describe()?

@panoply
Copy link
Author

panoply commented Aug 9, 2022

@novemberborn I will fork this week and have a look. I don't want to impede too much on the current approaches. I think a good starting point is changing the display style which is an easy enough refactor (https://github.com/avajs/ava/blob/main/lib/snapshot-manager.js#L88-L101). Extending t.describe() would likely be the most ideal for my use cases but I wouldn't want that to be the driving force of major overhauls.

@Lorenzobattistela
Copy link

Is this a good first Issue for a new contributor? I am intersted in contributing with this Issue, are there any material or resources for better understanding of the fixes for this issue? As far as I understand, changes should be made in the blockquotes display style.

If its okay, I am willing to pick up this issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants