-
Notifications
You must be signed in to change notification settings - Fork 6.8k
chore(tooltip): unit test failures #8385
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
Conversation
crisbeto
commented
Nov 12, 2017
- Fixes a regression in the tooltip that was introduced by 0719c38 which caused the tests to fail.
- Avoids issues in the future where failures in some tooltip tests can cause Jasmine to throw the browser into an infinite loop by trying to stringify a circular object.
@@ -261,7 +262,7 @@ export class MatTooltip implements OnDestroy { | |||
this._tooltipInstance = overlayRef.attach(portal).instance; | |||
|
|||
// Dispose of the tooltip when the overlay is detached. | |||
overlayRef.detachments().subscribe(() => { | |||
merge(this._tooltipInstance!.afterHidden(), overlayRef.detachments()).subscribe(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the !
up to the assignment for tooltipInstance
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TS isn't smart enough to figure it out when it's being assigned to a property, it only works for variables.
src/lib/tooltip/tooltip.spec.ts
Outdated
@@ -80,7 +80,7 @@ describe('MatTooltip', () => { | |||
}); | |||
|
|||
it('should show and hide the tooltip', fakeAsync(() => { | |||
expect(tooltipDirective._tooltipInstance).toBeUndefined(); | |||
expect(!!tooltipDirective._tooltipInstance).toBe(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make it just one place that does this? Something like a method getTooltipInstance
? That way it's easy to add a comment in one place that explains it rather than repeating !!
without explanation
* Fixes a regression in the tooltip that was introduced by 0719c38 which caused the tests to fail. * Avoids issues in the future where failures in some tooltip tests can cause Jasmine to throw the browser into an infinite loop by trying to stringify a circular object.
95b4ea3
to
223c49b
Compare
Addressed the feedback @jelbourn. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |