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 tests fail with --coverage due to Istanbul instrumentation. #85

Closed
brianmcallister opened this issue Oct 19, 2017 · 12 comments
Closed

Comments

@brianmcallister
Copy link
Contributor

Not 100% sure if this is an issue with this project, but it appears as though when I run Jest with --coverage, Istanbul instrumentation ends up in the compiled code, which fails my snapshot tests. Here's a look at the Jest output:

  ● LoginPageComponent › should render and match snapshot

    expect(value).toMatchSnapshot()
    
    Received value does not match stored snapshot 1.
    
    - Snapshot
    + Received
    
    @@ -29,12 +29,11 @@
              >
                
            
                <app-input
                  label="Username"
    -             ng-reflect-change="function (value) {
    -              "
    +             ng-reflect-change="function (value) {/* istanbul "
                  ng-reflect-label="Username"
                >
                  <div
                    class="input"
                  >

I found this issue in the Jest repository, and it seems related, though the person reporting the issue was using a different set of libraries, and it looks like he was testing a React application.

Let me know if there's more information I can provide.

@thymikee
Copy link
Owner

Uh oh. maybe we should remove the ng-reflect-change entirely? Don't think it's super necessary to snapshot this.

@brianmcallister
Copy link
Contributor Author

I'm a bit new to Angular, but it seems like it's something that you would want, because then the function that you've passed your handler to ends up in the snapshot. I am testing that the functionality works in a separate unit test, but I think there might be some value in "testing" that you've passed the correct props to the correct components.

Again though, very new to Angular so I might be off base there.

@thymikee
Copy link
Owner

To me function (value) {/* istanbul doesn't reflect any value, but I may be wrong. I'm not currently using Angular, so it's hard for me to tell. It may be helpful, but definitely not in this form. We could improve the snapshot serializer to change this into some kind of token, because we don't want the function implementation written to the disk, right?

For now, I'm cool with accepting a PR which filters out any valid /* istanbul calls like this.

@brianmcallister
Copy link
Contributor Author

Good point about not needing the function implementation, I definitely agree with that (especially since I unit test those anyway). Maybe something like ng-reflect-change=[Function]... something along those lines.

What would be the correct place to implement a filter like that? Seems like AngularSnapshotSerializer.js, though it seems likely that Istanbul adds those comments after the component gets serialized.

@thymikee
Copy link
Owner

That's a right place to add this. Something similar was implemented in pretty-format which formats this code inside Jest serializers, so you can check st the implementation there if you like :)

@brianmcallister
Copy link
Contributor Author

OK, sounds great, thanks. I'll try and carve out some time in the next week or two to work through this.

@thymikee
Copy link
Owner

Awesome, thank you ❤️

@thymikee
Copy link
Owner

@brianmcallister have tried this? jestjs/jest#1740 (comment)

@brianmcallister
Copy link
Contributor Author

Unfortunately, it looks like the betas aren't working at all (at least for me), instead, they just throw errors on every test.

I did dig into this a bit, and this looks like an issue with pretty-format instead of this repository. Although pretty-format does support plugins (which is probably the cleanest way to handle this, maybe?), I don't see an easy way to add a plugin when adding a custom snapshot serializer.

I don't think it makes too much sense to attempt to patch pretty-format here since I think this issue might be specific to Angular.

I'll keep trying the jest betas as they're published and see if that helps.

@thymikee
Copy link
Owner

When the next beta is up (something above 21.3.0-beta.4), add "testEnvironment": "jsdom" to your Jest config and it should work. Or add setup/teardown methods to current testEnvironment.js (by modifying node_modules or linking the preset) if you want to test it now.

@sylvainleduby
Copy link

Same issue here with [email protected] and [email protected] (and [email protected]):

                ng-reflect-resolve-title-component="function (type) {
    -           retu"
    +           /* i"

"Resolved" by increasing the length of the parameter of our function to eventType to move Istanbul's comment out of the string used in the snapshot. But it does not seem robust at all.
Any better (real) solution?

@ahnpnl
Copy link
Collaborator

ahnpnl commented Sep 14, 2019

Please try with 7.1.1, the preset now has new serializers to strip out some unnecessary things from snapshot. Feel free to report if you encounter new issues.

@ahnpnl ahnpnl closed this as completed Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants