-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
base: master
Are you sure you want to change the base?
Build-in Ember-Exam #426
Conversation
b2ed63c
to
d685867
Compare
This reads great, thank you for sharing this RFC! I whole-heartedly agree with the points you already made in favor of embracing One particular feature of What are your and others' thoughts on it? |
@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. |
Another issue I'd like to call out is 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 If we were to integrate I'm not certain which option I prefer from the |
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 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. |
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. Update: Still getting timeout issues without ember-exam, so likely unrelated and shouldn't hinder this RFC |
@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. -- In general, I am very much on-board with |
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. |
Rendered