Fix state leakage in test-amp-user-notification.js #10200
Conversation
| }); | ||
|
|
||
| afterEach(() => { | ||
| if (storageMock) { |
There was a problem hiding this comment.
We no longer verify with storageMock? I see we create storageMock in every test, but only call verify() in some of them.
There was a problem hiding this comment.
Good catch. Added to all cases.
We can't do in afterEach because for some cases we have to wait a promise to resolve :-(
Also, somehow having assertion in afterEach causes Mocha to report incorrect total test number.
There was a problem hiding this comment.
Also, somehow having assertion in afterEach causes Mocha to report incorrect total test number.
I get this one
We can't do in afterEach because for some cases we have to wait a promise to resolve :-(
But afterEach will be called after test promise resolved right?
There was a problem hiding this comment.
yes, but then you need to return impl.storagePromise_ in each test, which isn't nice.
| return impl.show().then(() => { | ||
| expect(el).to.have.class('amp-active'); | ||
| expect(addToFixedLayerStub).to.be.calledOnce; | ||
| expect(addToFixedLayerStub.getCall(0).args[0]).to.equal(el); |
There was a problem hiding this comment.
what here and below? do we need verify() as well?
There was a problem hiding this comment.
only needed when there is storageMock.expects
* describes * Fix flaky test-amp-user-notification.js * verify
Blocking #10074