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

Build-in Ember-Exam #426

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cibernox
Copy link
Contributor

@jayjayjpg
Copy link
Member

jayjayjpg commented Jan 3, 2019

This reads great, thank you for sharing this RFC! I whole-heartedly agree with the points you already made in favor of embracing ember-exam in Ember's testing workflow by default for improving testing speed.

One particular feature of ember-exam I find incredibly useful is the randomization of testing order which quickly reveals order dependency between tests. In my opinion using the --random flag by default could help many developers to write more reliable tests from the get go by highlighting if setup and cleanup of state-altering tests has been done correctly.

What are your and others' thoughts on it?

@bendemboski
Copy link
Collaborator

@jessica-jordan I'm finding myself mildly resistant to the idea of randomizing test order by default. I definitely see the value in the feature, but I also think there a lot of value in having deterministic tests. It's very disruptive to have my PR build pass and then the master build fail after merging, or to do something trivial like update the README of my long-dormant maintenance-mode addon and see the build fail. This can happen due to timing conditions or dependency drift or whatever, but I'm reluctant to add another significantly-likely cause of such issues.

In my experience, test contamination is very rarely hiding an actual production bug (perhaps other have a different experience?), so randomizing the order is really about finding bugs in the tests, not the production code. When weighed against the test suite being deterministic, to me determinism wins out. I also think that to newcomers to Ember, having a deterministic test suite is more valuable than education on test contamination.

So my vote is to keep the order randomization as opt-in, but I think it could definitely make sense to highlight it a good practice in the documentation, or something similar.

@bendemboski
Copy link
Collaborator

bendemboski commented Jan 6, 2019

Another issue I'd like to call out is ember-electron's integration. ember-electron implements a custom electron:test command that runs tests in an electron instance, and it's been a bit of a maintenance headache for a number of reasons (many of which aren't affected at all by this RFC).

It's complex enough that I can't rattle off the implications of this RFC on it with 100% certainty without doing some research, but I believe the following is true.

If we went with the option of adding ember-exam to the blueprint, ember-electron's integration would be unaffected, but of course wouldn't integrate with any of the ember-exam features, which is not ergonomically ideal, but the developer ergonomics of ember-electron are already pretty far from ideal, and I think this would be a pretty small negative contribution to the whole percentage-wise.

If we were to integrate ember-exam's functionality into the relevant libraries, then we'd almost certainly have to do some work to update ember-electron's integration, but then ember-electron tests could probably leverage/respect the new functionality, and there's a chance it might even simplify the integration and make the maintenance less of a pain in my, uh, neck 😄

I'm not certain which option I prefer from the ember-electron standpoint -- I think it would depend on lot on the specifics of the second option -- but I'd very much appreciate being looped in on any more detailed discussions of how this might work so I can represent the ember-electron angle (and also considering a second customized testing command seems like very valuable data when designing an integrated solution).

@mehulkar
Copy link
Contributor

mehulkar commented Jan 7, 2019

I like RSpec’s implementation of this. Randomization is controlled by a command line flag that can also be permanently set in a .rc file. Each test run emits a seed value that can be specified to get a deterministic order. For example rspc spec/ —random —seed 12445.

I agree that test contamination issues are usually not production failures, but random test failures are bad for productivity.

That said, I would hope that this functionality would be built into Qunit / Mocha and controlled with their Config rather than an ember specific feature.

@brandynbennett
Copy link

brandynbennett commented Jan 8, 2019

There are some frustrating browser timeout issues that have come up that I think should be resolved before this becomes part of the default Ember blueprint.
For example: ember-cli/ember-exam#131
I recently had to remove ember-exam from our CI build and do regular ember test, because of sporadic timeout test failures.

Update: Still getting timeout issues without ember-exam, so likely unrelated and shouldn't hinder this RFC

@stefanpenner
Copy link
Member

stefanpenner commented Jan 8, 2019

@step2yeung / @choheekim have been working on: improved quality of the code/tests and also improved load balancing.

We hope for this work to land in the next few weeks, and I think the quality improvements they are working on are a pre-req to this being part of ember-cli.

PR: ember-cli/ember-exam#136

--

In general, I am very much on-board with ember exam becoming ember test in the future.

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

It seems like there is some solid interest in this and I can see the potential benefits of making this first-class. I'll see if we can get some more proper review on it and get a yay or nay on it soon.

@wagenet wagenet added S-Proposed In the Proposed Stage T-ember-cli RFCs that impact the ember-cli library S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Champion S-Exploring In the Exploring RFC Stage T-ember-cli RFCs that impact the ember-cli library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants