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.

Sign up for GitHub

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

Jump to bottom

Reset module variable after all tests in module are completed. #260

Merged
merged 3 commits into from
Apr 10, 2017
Merged

Reset module variable after all tests in module are completed. #260

merged 3 commits into from
Apr 10, 2017

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Apr 4, 2017

No description provided.

trentmwillis
trentmwillis requested changes Apr 4, 2017
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

One small thing, otherwise seems good to me.

lib/ember-qunit/qunit-module.js Outdated

qunitModule(module.name, {
before() {
module = new Constructor(name, description, callbacks);
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a comment that we don't want to assign this on this to avoid allowing the test to access it?

Copy link
Member Author

Choose a reason for hiding this comment

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

ya, sounds good

Copy link
Member Author

Choose a reason for hiding this comment

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

updated with a comment now

@rwjblue rwjblue mentioned this pull request Apr 4, 2017
Merged
trentmwillis
trentmwillis requested changes Apr 4, 2017
View reviewed changes
lib/ember-qunit/qunit-module.js Outdated
var module;

qunitModule(name, {
before() {
Copy link
Member

Choose a reason for hiding this comment

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

This will also need to support user-defined before/after hooks, otherwise we potentially break existing tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

D'OH! Good point, can't believe I forgot that 😢

@rwjblue
Copy link
Member Author

rwjblue commented Apr 6, 2017

Refactor looks good, very similar to what I did in the new API PR...

@trentmwillis
Copy link
Member

Due to the issues with a similar change in the 0.4.x branch, I'm going to hold on merging this until I can verify it works against a couple apps

trentmwillis
trentmwillis approved these changes Apr 6, 2017
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

Okay, tested the changes against ember-cli-addon-search, travis-web, and ember-observer. All passed just fine.

@trentmwillis
Copy link
Member

Okay had some minor updates around tests and the name used by the module. @rwjblue would appreciate one more quick review before merging.

@trentmwillis trentmwillis merged commit 94d222e into master Apr 10, 2017
@trentmwillis trentmwillis deleted the clear-module-after-tests branch April 10, 2017 16:09
@trentmwillis trentmwillis mentioned this pull request Apr 11, 2017
Merged
@bendemboski bendemboski mentioned this pull request Apr 12, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trentmwillis trentmwillis trentmwillis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rwjblue @trentmwillis