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

Improve WebChannel mocking #3647

Merged
merged 2 commits into from
Nov 16, 2021

Conversation

mstange
Copy link
Contributor

@mstange mstange commented Nov 9, 2021

This PR has two commits which improve the mock WebChannel which we use in tests: The code is moved to a different file, it no longer breaks other uses of window.dispatchEvent, and it adds a way to clean up after the test.

@mstange mstange requested a review from canova November 9, 2021 21:39
@mstange mstange self-assigned this Nov 9, 2021
@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #3647 (8d06672) into main (bead77f) will decrease coverage by 0.02%.
The diff coverage is 78.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3647      +/-   ##
==========================================
- Coverage   88.90%   88.87%   -0.03%     
==========================================
  Files         258      258              
  Lines       21498    21542      +44     
  Branches     5494     5504      +10     
==========================================
+ Hits        19112    19146      +34     
- Misses       2212     2222      +10     
  Partials      174      174              
Impacted Files Coverage Δ
src/test/fixtures/mocks/web-channel.js 84.81% <78.26%> (-9.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bead77f...8d06672. Read the comment docs.

@mstange mstange requested review from julienw and removed request for canova November 15, 2021 16:05
Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me for calling the original addEventListener/removeEventListener/dispatchEvent functions, but please double check if all these restoreOriginals calls are useful, as I suspect they're not. If they're not useful can you please remove them?

Thanks!

@@ -124,5 +127,7 @@ describe('app/DragAndDrop', () => {

fireEvent.dragLeave(dragAndDrop);
expect(overlay.classList).not.toContain('dragging');

restoreOriginals();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to add all these restoreOriginals calls, because jest.spyOn spys are automatically removed after each test. See

jest.resetAllMocks();
jest.restoreAllMocks();
jest.clearAllTimers();
jest.useRealTimers();

(historical note: in the past we used to do this with some jest configuration, but this wasn't working properly, see jestjs/jest#9896 especially. That's why we're doing it manually in the setup file. I don't know if that improved.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, perfect! That should make the patch a lot simpler.

This means that these functions will be usable from UrlManager.test.js.
I'll make use of that in a future PR.
The WebChannel mocking now calls the original functions for
addEventListener, removeEventListener, and dispatchEvent. This means that
it won't interfere with other uses of these APIs.

One example of such a use is the popstate event: UrlManager.test.js has
a test which makes sure that the URL state is restored when going
backwards in history. Using a mocked WebChannel in that test broke it
because the popstate event was no longer being dispatched.
@mstange mstange force-pushed the less-invasive-mock-webchannel branch from d0af54c to 8d06672 Compare November 16, 2021 21:34
@mstange mstange merged commit 1025f91 into firefox-devtools:main Nov 16, 2021
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

Successfully merging this pull request may close these issues.

2 participants