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

Fix Beta/Canary Tests #404

Merged
merged 13 commits into from Apr 19, 2021
Merged

Conversation

alexlafroscia
Copy link
Contributor

@alexlafroscia alexlafroscia commented Apr 3, 2021

Changes proposed in this pull request

The tests for #403 show the ember-beta and ember-canary builds timing out. This should address those problems and get them working again!

It also fixes some warnings related to Ember 4.0 that are printed when running the tests.

A few code mods were used to migrate to better-recommended approaches to things:

The qunit-dom change was made to resolve an issue where find('*') was used to check the text content of the whole "template" for an integration test, but that syntax isn't actually valid. I thought that qunit-dom might help. That wasn't quite the answer, as I still had to go remove the '*' argument that was code-modded into the qunit-dom syntax, but the qunit-dom error made it more clear what was going wrong. Since the syntax is cleaner and the tests read more easily with them using that library, I figured I would leave that in this PR.

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Apr 3, 2021

I think the actual culprit here was the ember-cli-babel version; it was odd that in CI, nothing was bring printed for the test output in the Beta and Canary builds. I think the problem was too many warnings being printed to the browser, stemming from the outdated ember-cli-babel version transpiling code to use the Ember global, which is now deprecated in those Ember versions.

If the rest of the changes here should be removed, I can pull them into a separate PR or ditch them altogether.

I'm also happy to take that clean-up further and remove the use of the action helper in the tests, since that's not something that really fits with the current Ember recommendations.

@alexlafroscia
Copy link
Contributor Author

Okay, so the ember-qunit and ember-resolver respectively resolve some of the deprecation warnings about the Ember global.

To get the rest of them, we need to upgrade more dependencies that depend themselves on ember-cli-babel; that's the "root cause" of some of these issues.

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Apr 4, 2021

^ I'm hoping that cleaning up more of the deprecation warnings will show us whatever is causing the embroider test case to not show any kind of results log, now that the beta and canary tests are passing!

Edit: This seems to be the error hanging up Embroider:

not ok 1 Chrome 89.0 - [undefined ms] - Global error: Uncaught Error: an unsupported module was defined, expected define(id, deps, module) instead got: 1 arguments to define` at http://localhost:7357/assets/vendor.js, line 67

@alexlafroscia
Copy link
Contributor Author

With those last dependency updates, I was able to get the Embroider build to pass locally... 🤞 that it passes in CI, too!

@alexlafroscia
Copy link
Contributor Author

Same issue as before is still happening in CI, but Embroider installed locally seems to work fine... I don't get it 🤷

@alexlafroscia
Copy link
Contributor Author

Still working on reproducing that specific define-related bug locally...

If I just run the normal Embroider build locally (by follow it's normal installation steps) it works fine.

If I try to run it through ember-try locally, though, I always get hung up on an error of this flavor, though which dependency it complains about changes

CleanShot 2021-04-04 at 09 55 11@2x

@alexlafroscia
Copy link
Contributor Author

Welp, I thought that a hail-Mary attempt at regenerating the whole lockfile might fix things by upgrading some deep-down version of a package that might be causing the problems, but... Apparently not!

@alexlafroscia
Copy link
Contributor Author

I'm going to try the Embroider setup from #403 just to see if that, for some reason, helps...

@alexlafroscia
Copy link
Contributor Author

Nope... Same problem 😡

@alexlafroscia
Copy link
Contributor Author

Okay, I'm going to back off and take a different approach here. I think there are some CI configuration improvements that could be made, including allowing the Beta and Canary builds to fail (at least for now, where it seems like it's something on Ember's end rather than on this package's end).

I'm going to open a separate PR to address that clean-up, then revisit after that.

I tried again to get the Embroider tests to fail locally but... they just don't. That's making me think it might have something to do with the CI environment itself. Nothing seems strange but here are some outdated packages (like ember-cli-eslint) that could be removed to get the configuration more inline with the modern standard.

This was referenced Apr 5, 2021
* Upgrade package
* Use `hbs` template tag from that package instead
* Set the Ember Edition in `package.json`
* Set all optional features to Octane default values
* Remove single-use, classic-style components in favor of inlining an Octane-style equivalent into the test itself
This prevents accessing the Ember global in the 3.27+ version of Ember, in which this behavior is deprecated
@alexlafroscia alexlafroscia force-pushed the beta-compatibility branch 2 times, most recently from 16b5d4e to 5f8bc3f Compare April 12, 2021 19:01
@snewcomer
Copy link
Contributor

Is this current with the main branch? Should we update/merge with the ember-try failures as welll?

@snewcomer snewcomer merged commit b253d2e into DockYard:master Apr 19, 2021
@snewcomer snewcomer mentioned this pull request Apr 19, 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
2 participants