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

Configure @embroider/test-setup #430

Merged
merged 2 commits into from
Jun 16, 2021

Conversation

alexlafroscia
Copy link
Contributor

This configures ember-concurrency to run the test suite against Embroider builds using the recommended approach (the @embroider/test-setup package).

This is an alternative to the approach taken in #368, which achieves the same goal manually; the package essentially does the same thing, but we abstract the "how" away to Embroider itself (which makes us more resilient to changes).

I configured the two Embroider jobs to allow failures, so that getting them to actually work can be done outside of the process of just seeing whether they pass or not.

@alexlafroscia alexlafroscia force-pushed the embroider-test-setup branch 4 times, most recently from 3fc8093 to d7ab45c Compare June 2, 2021 20:40
Copy link
Collaborator

@maxfierke maxfierke left a comment

Choose a reason for hiding this comment

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

Looks good. I think for now, given the timeouts on embroider-safe and the failures in embroider-optimized it would be good to leave those out of the GitHub Actions CI run for now until they're fixed, as I don't want to hold up PRs on those or waste Actions cycles

and then we can enable them when they're passing

@alexlafroscia alexlafroscia force-pushed the embroider-test-setup branch from d7ab45c to 41896f6 Compare June 16, 2021 14:23
@alexlafroscia
Copy link
Contributor Author

Sorry that took me so long to get back around to! I rebased the PR to remove the merge conflict and removed the Embroider tests from running as part of CI.

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Jun 16, 2021

Embroider 0.42.0 dropped yesterday, which no longer supports Node 10 (which this project uses). I couldn't bump this PR to use it because of the engines mis-match.

Is there a reason to support Node 10 these days, where the LTS release is now Node 14?

Maybe it would be good to (in a separate PR) update the support policy to 12/14/16 as Embroider is doing now?

Realistically the Node version policy is irrelevant for this addon, since there really isn't anything happening at the build-time level here.

@maxfierke
Copy link
Collaborator

Embroider 0.42.0 dropped yesterday, which no longer supports Node 10 (which this project uses). I couldn't bump this PR to use it because of the engines mis-match.

Is there a reason to support Node 10 these days, where the LTS release is now Node 14?

Maybe it would be good to (in a separate PR) update the support policy to 12/14/16 as Embroider is doing now?

Realistically the Node version policy is irrelevant for this addon, since there really isn't anything happening at the build-time level here.

heh, this is a bit of a philosophical debate I suppose. I personally don't have an issue bumping the Node version without a major version bump. We've done it before, we don't really hold ourselves to a specific one anyway, and I never know anything about any of the differences in the Node versions anyway. However, it's definitely come up before that people have had issues with that.

I think what I may do is merge this PR as-is and do some thinking about adding something of a policy in the README or somewhere else that indicates we consider Node version to be out-of-scope for SemVer given the general lack-of-bearing it has on ember-concurrency specifically.

Then we can do a bump to 0.42 separately once that's ironed out. (There's also a case to be made for tagging a version 3.0.0 as the next release as there's certainly some pre-Octane baggage we could probably start dropping in the next few months anyway, but there's maybe a couple more features I'd like to land in 2.x, so will maybe sit on that.)

@alexlafroscia
Copy link
Contributor Author

alexlafroscia commented Jun 16, 2021

Sounds good! I wasn't even necessarily thinking of the Node version bump not being a breaking change -- it's probably safer to make is a major bump, but as long as the CHANGELOG reflects that that's what broke, it's easy for people to determine if they can safely upgrade or if something needs to be done!

Copy link
Collaborator

@maxfierke maxfierke left a comment

Choose a reason for hiding this comment

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

:octocat: thanks again Alex, this looks great!

@maxfierke maxfierke merged commit 893669c into machty:master Jun 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