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
from

Conversation

Projects
None yet
6 participants
@cibernox
Copy link
Contributor

cibernox commented Dec 24, 2018

Rendered

@cibernox cibernox force-pushed the cibernox:build-in-ember-exam branch from b2ed63c to d685867 Dec 24, 2018

@jessica-jordan jessica-jordan referenced this pull request Jan 1, 2019

Merged

Ember Times - Issue No. 79 #3750

7 of 16 tasks complete
@jessica-jordan

This comment has been minimized.

Copy link
Member

jessica-jordan 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

This comment has been minimized.

Copy link

bendemboski commented Jan 6, 2019

@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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

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

This comment has been minimized.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment