-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add compatibility for embroider #191
Conversation
Feature more modern examples, update for breaking changes in #191
The new release v0.7.0 should be compatible now, through simonihmig/ember-window-mock#191
hooks.beforeEach(() => _setCurrentHandler(mockProxyHandler)) | ||
hooks.afterEach(() => { | ||
reset(); | ||
_setCurrentHandler(null); |
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.
@simonihmig - Sorry for the late comment, I'm curious if you think this should be a stack instead of always resetting to null
?
Specifically, should we reset to null
or to whatever the value was before? What happens if you do:
module('foo', function(hooks) {
setWindowMock(hooks);
module('nested thing', function(hooks) {
setWindowMock(hooks);
});
test('something that requires the window mock', function() {
// ...snip...
});
});
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.
It is also vaguely weird that it would reset with null
. I would think that you would want to call _setCurrentHandler()
which would reset to defaultHandler
(AFAICT calling with null
will make any window
access fail).
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.
@rwjblue thanks for the review and the suggestions, I think they make totally sense! Especially that null
thing was stupid, apparently I thought this would reset to defaultHandler
, which you are right it does not. I should add some better test coverage, for what state setupWindowMock
leaves behind afterwards, especially for that nested usage...
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.
For the record, as of 0.7.1
it no longer resets to null
; it now returns to the default handler after each test.
This adds better test coverage for calls to `setupWindowMock()`, specifically the nested usage discussed with @rwjblue in #191#discussion_r454379796. It turned out that the current implementation (after being fixed by @alexlafroscia in #201) already supports the nested case properly, as the `mockHandler` is set up in the `beforeEach` hook, so as long as there is any nested `setupWindowMock()` the window object will be mocked, and only when a test is not somehow wrapped in a setup call will it see the unmocked window.
Fixes #107
Follows the suggestions laid out in https://github.com/embroider-build/embroider/blob/master/packages/compat/src/compat-adapters/ember-window-mock.ts:
import window from 'ember-window-mock';
ember-window-mock/test-support
, including the setup function:import { setupWindowMock } from 'ember-window-mock/test-support';
.This is a breaking change
ember-window-mock
module has some Proxy related code, so you can turn on and off the mocking behavior at runtime. Which means thatimport window from 'ember-window-mock';
gives you now a Proxy-wrapped window object even outside tests (that's new!), which however passes everything through to the original window unlesssetupWindowMock()
is called. So apart from bugs, this should not make a difference, unless you are developing on IE11 😱window
global directly.This also adds an Embroider test scenario, which however was passing even without the refactorings above, as the build included all the mocking code the tests expect. Now mocking code is not included anymore outside of tests, which I checked manually, but which we don't have any kind of test coverage for.
@ef4 in case you have a minute left, a quick review would be highly appreciated! 🙂
Hope this is reasonable so far and follows the (module) semantics that embroider expects!
/cc @kiwiupover