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

Migrate acceptance tests to use destroy-app helper #84

Merged
merged 1 commit into from
Sep 21, 2015

Conversation

blimmer
Copy link
Contributor

@blimmer blimmer commented Sep 20, 2015

The next version of Ember CLI will have a new destroy app helper, similiar to the start-app helper that already exists. I want to write a migrator to help automatically move everyone over to this new syntax.

Additionally, I link to a place where users can learn how to create this helper and use it before they're able to upgrade to Ember CLI 1.13.9 (yet to be released).

Introduced in Ember CLI 1.13.9.
@blimmer blimmer changed the title [WIP] supporting a destroy-app helper. Migrate acceptance tests to use destroy-app helper Sep 21, 2015
@rwjblue
Copy link
Contributor

rwjblue commented Sep 21, 2015

Just a note: using this.application = startApp() (vs just using a local variable) was only changed a version or two ago.

If the test in question does not use this.application = startApp() (it is using application = startApp() instead), should we update the usage to the new format?

@blimmer
Copy link
Contributor Author

blimmer commented Sep 21, 2015

@rwjblue I wonder if that should be covered in the more generic qunit transforms rather than here?

@abuiles
Copy link
Owner

abuiles commented Sep 21, 2015

@blimmer thanks for this! Probably we can add it here? The old transform was to help people migrate to QUnit 2 syntax.

@blimmer
Copy link
Contributor Author

blimmer commented Sep 21, 2015

I just worry that that introduces pretty big scope creep to this story. Changing that would also mean that we would have to parse the rest of the file to ensure we change any other references to this.application to application to ensure we don't break people's tests. That change feels different from the goal of this formula.

That said, if you guys disagree, I can take a look at adding that functionality as time permits.

@abuiles
Copy link
Owner

abuiles commented Sep 21, 2015

@blimmer Makes sense, let's then create another formula for that? I'll merge this in and release, thank you very much!

@abuiles
Copy link
Owner

abuiles commented Sep 21, 2015

@blimmer I will hold merging and release until ember-cli gets released, @rwjblue @stefanpenner can we sync on that, ping me before release so I'll release this new command too.

@knownasilya
Copy link
Collaborator

Sounds like we need a min version option for commands, making the command not run if the version of Ember CLI is less then that min and giving the user a nice message.

@blimmer
Copy link
Contributor Author

blimmer commented Sep 21, 2015

The gist I link to if the file isn't present explains how to get this functionality before the new Ember-CLI version is released. This is pretty crucial to another fix for ember-cli-mirage, and some might want to manually place the destroy-app.js helper before Ember CLI 1.13.9.

However, if the standard is to not release new ember watson before the other PR is mainlined, we can definitely stick to that behavior.

@abuiles
Copy link
Owner

abuiles commented Sep 21, 2015

@blimmer you are right, we don't need to hold this until ember-cli release. :shipit:

abuiles added a commit that referenced this pull request Sep 21, 2015
Migrate acceptance tests to use destroy-app helper
@abuiles abuiles merged commit 394cfef into abuiles:master Sep 21, 2015
@abuiles
Copy link
Owner

abuiles commented Sep 21, 2015

@blimmer released 0.6.6 👍 thanks!

@rwjblue
Copy link
Contributor

rwjblue commented Sep 21, 2015

Changing that would also mean that we would have to parse the rest of the file to ensure we change any other references to this.application to application to ensure we don't break people's tests.

Agreed, but this migration isn't helpful at all (and might actually result in errors when running tests) if this.application is not set, right?

@abuiles
Copy link
Owner

abuiles commented Sep 21, 2015

@rwjblue
Copy link
Contributor

rwjblue commented Sep 21, 2015

@abuiles - I don't think so. Once folks update ember-cli they will have that file (so that guard will not return early), but their tests will not be using this.application.

I did try to review the transform code again, and I think my concerns are unfounded since the transform will use whatever the right hand assignment is of startApp() call (which means app = startApp(), this.foo = startApp(), etc should work fine).

@abuiles
Copy link
Owner

abuiles commented Sep 21, 2015

@rwjblue awesome! @blimmer can you confirm?

@blimmer
Copy link
Contributor Author

blimmer commented Sep 21, 2015

Yes, the transform will work no matter what you use to store the application object. I explicitly test for it for mocha and qunit. here's the relevant code.

@blimmer blimmer deleted the destroy-app-helper branch April 25, 2019 22:28
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.

4 participants