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

Issue with spying on CommonModule provider by default #735

Closed
GerkinDev opened this issue Jun 21, 2021 · 12 comments · Fixed by #737
Closed

Issue with spying on CommonModule provider by default #735

GerkinDev opened this issue Jun 21, 2021 · 12 comments · Fixed by #737

Comments

@GerkinDev
Copy link

GerkinDev commented Jun 21, 2021

Hi there ! I hope you're doing well, and I see you continue to do an amazing job on this lib.

I'm posting this issue because I've just migrated my app to ng12, and latest ng-mocks, and now, some things are broken. I don't know if this is directly related to ng-mocks itself or not, so I'd gladly take any indication.

I used to spy on some CommonModule providers to check my calls, and it used to work fine:


Here is the thing I had before:

package.json

{
    "@angular/common": "~11.2.10",
    "ng-mocks": "^11.10.1"
}

setupJest.ts

ngMocks.defaultMock( DomSanitizer, ( _s, injector ) => {
	const sanitizer = new ɵDomSanitizerImpl( injector.get( DOCUMENT ) );
	return new ( class MockDomSanitizer implements DomSanitizer {
		public sanitize = jest.fn( sanitizer.sanitize.bind( sanitizer ) );
		public bypassSecurityTrustHtml = jest.fn( sanitizer.bypassSecurityTrustHtml.bind( sanitizer ) );
		public bypassSecurityTrustStyle = jest.fn( sanitizer.bypassSecurityTrustStyle.bind( sanitizer ) );
		public bypassSecurityTrustScript = jest.fn( sanitizer.bypassSecurityTrustScript.bind( sanitizer ) );
		public bypassSecurityTrustUrl = jest.fn( sanitizer.bypassSecurityTrustUrl.bind( sanitizer ) );
		public bypassSecurityTrustResourceUrl = jest.fn( sanitizer.bypassSecurityTrustResourceUrl.bind( sanitizer ) );
	} )();
} );

After the update:

package.json

{
    "@angular/common": "~12.0.5",
    "ng-mocks": "^12.1.2"
}

But now, all of my tests asserting on DomSanitizer methods calls fails. Seeing this RTFM, I've tried this:

setupJest.ts

ngMocks.globalMock( DomSanitizer );
ngMocks.defaultMock( DomSanitizer, sanitizer => {
	jest.spyOn( sanitizer, 'sanitize' );
	jest.spyOn( sanitizer, 'bypassSecurityTrustHtml' );
	jest.spyOn( sanitizer, 'bypassSecurityTrustStyle' );
	jest.spyOn( sanitizer, 'bypassSecurityTrustScript' );
	jest.spyOn( sanitizer, 'bypassSecurityTrustUrl' );
	jest.spyOn( sanitizer, 'bypassSecurityTrustResourceUrl' );
	return sanitizer;
} );

But still no luck.

So, have you an idea on what I should do ? Am I forced to mock it with MockInstance in each of my files ?

🍻 cheers !


Footnote:

I guess this is related, but mocking ComponentFactoryResolver provider directly in some MockBuilder's call chains seems to not work either:

	beforeEach( () => MockBuilder( DynamicComponentHostComponent )
		.provide( { provide: ComponentFactoryResolver, useFactory: () => createSpyObj<ComponentFactoryResolver>( [ 'resolveComponentFactory' ] ) } ) );
@GerkinDev GerkinDev changed the title Issue with spying on CommonModule by default Issue with spying on CommonModule provider by default Jun 21, 2021
@satanTime
Copy link
Member

Hi @GerkinDev,

I hope you are fine.

There is a recent change about DomSanitizer. The problem was that it is like NgZone, highly likely nobody expects it to be a mock.

However, if someone is providing a mock - then a mock DomSanitizerhas to respect it.

I'll investigate it this week and provide a fix with the next release.

@GerkinDev
Copy link
Author

Alright! Is it the same thing for ComponentFactoryResolver?
Actually, about the dom sanitizer, I don't want to mock it, but to spy it. If you have better solutions, I would take it!

@satanTime
Copy link
Member

Ah true. And the resolver too :)

It was a fix to return mocks instead of real components based on current suite. The idea was to simplify popups / overlays testing.

Might you create a separate issue with an example about the resolver?

@satanTime
Copy link
Member

I've created an issue. Love the button to create an issue from a comment, github rocks!

@satanTime
Copy link
Member

Hi @GerkinDev,

only now I've read the full description :) I'm doing well, thanks for asking, yourself?

There are 2 cool things: the summer is finally here and today Github has sent your donation to my bank account,
so this weekend I plan to start with a glass of beer in honor of you and open source community.

Besides that, could you verify the fix? ng-mocks.zip

It requires the preliminary call of globalMock, as in your the 2nd example.

ngMocks.globalMock(DomSanitizer);

@GerkinDev
Copy link
Author

As mentioned in the other issue, I'm off for a few days, so I'll give you feedbacks soon.

satanTime added a commit to satanTime/ng-mocks that referenced this issue Jun 26, 2021
satanTime added a commit to satanTime/ng-mocks that referenced this issue Jun 26, 2021
satanTime added a commit to satanTime/ng-mocks that referenced this issue Jun 26, 2021
satanTime added a commit to satanTime/ng-mocks that referenced this issue Jun 26, 2021
@satanTime
Copy link
Member

Hi @GerkinDev,

no rush, as usually. There is a proper fix, I hope :)
ng-mocks.zip

satanTime added a commit to satanTime/ng-mocks that referenced this issue Jun 27, 2021
satanTime added a commit to satanTime/ng-mocks that referenced this issue Jun 28, 2021
satanTime added a commit to satanTime/ng-mocks that referenced this issue Jun 28, 2021
satanTime added a commit to satanTime/ng-mocks that referenced this issue Jun 28, 2021
@GerkinDev
Copy link
Author

Hi there ! So, I can confirm that the ZIP you sent fixes the problem here :) I would not say you're the best, but you're pretty close lol

@satanTime
Copy link
Member

Ahahahaha :) Thanks! Then I'll prepare a release next days.

I think, it should also fix the issue with ComponentFactory too, or be pretty close to solve it :)

@GerkinDev
Copy link
Author

I thought that too and I just alt+tabbed from my test, that was unfortunatelly non conclusive. I'm deep diving on it to see exactly what's the deal with it.

@satanTime
Copy link
Member

Ah, sad sad. Then, let's discuss it in #736

@satanTime
Copy link
Member

v12.2.0 has been released and contains a fix for the issue. Feel free to reopen the issue or to submit a new one if you meet any problems.

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

Successfully merging a pull request may close this issue.

2 participants