ngneat / spectator

🦊 🚀 A Powerful Tool to Simplify Your Angular Tests
https://ngneat.github.io/spectator
MIT License
2.07k stars 177 forks source link

Possible leakage between tests found #429

Open YoeriNijs opened 3 years ago

YoeriNijs commented 3 years ago

I'm submitting a...


[x] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request
[ ] Other... Please describe:

Current behavior

It seems that there is some memory leakage between unit tests when we are testing the Angular cdk dialog. It is possible that this has something to do with the cdk itself (https://github.com/angular/components/issues/22292), but I have too little experience with this to verify this.

Maybe I am just wrong and there is always some state left when using mocks, but it is important to note that the memory leakage exist since Angular 9. On 8, we did not have this problem in our dialog unit tests with the same setup (sort of). Furthermore, it is important to note that this issue for now is only reproducible with Spectator. Hence, my assumption is that is has something to do with how Spectator handles mocks, but we need some investigation for this.

Expected behavior

Unit tests should not keep state between them.

Minimal reproduction of the problem with instructions

As you see here in this example, I am just overwriting the injected value. After that, I am just performing a detectChanges. I am aware of the fact that you do not want to do this, because in my view you should provide the data via the mock itself, but this is just to illustrate that it keeps state.

https://github.com/YoeriNijs/angular-cdk-dialog-bug/blob/master/src/app/dialog/dialog.component.spec.ts

Just run ng test. I am just using Karma here since it is the default stack.

Screenshot 2021-04-15 at 10 04 13

What is the motivation / use case for changing the behavior?

We should never hold state between tests since it makes unit testing unreliable.

Environment


Angular version: 11.2.9


Browser:
- [x] Chrome (desktop) version 90.0.4430.72

Not tested on other browsers.

For Tooling issues:
- Node version: 12.16.3
- Platform:  Mac

Others:
IntelliJ, NPM
shonderdos commented 3 years ago

I've checked out your repo and have one general comment:

The second test should be Barry Boe when manipulating data and detectChanges (but it fails, because it is John Doe) is expected to fail. Calling createComponent() before the test will initialize the component which triggers NgOnInit(). Changing the property data in the test should not affect the template since NgOnInit() isn't triggered.

I'm not sure if that was intended but I wanted to point that out.

It is possible that this has something to do with the cdk itself (angular/components#22292), but I have too little experience with this to verify this.

I've been able to replicate the issues by using a custom @Injectable class without any dependency on @angular/material.

I've also recreated the test using TestBed and don't see the same behaviour. The third test should be John Doe again (but it fails, because it is Barry Boe) doesn't fail.

If I have to guess what the issue is, it would be that the mocked object is passed around by reference and we're not getting a fresh new copy everytime we call createComponent(). But that would have the be verified in the codebase.

I've created a PR with my investigation