rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Build-in Ember-Exam

Open cibernox opened this issue 5 years ago • 7 comments

Rendered

cibernox avatar Dec 24 '18 10:12 cibernox

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?

jayjayjpg avatar Jan 03 '19 22:01 jayjayjpg

@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 avatar Jan 06 '19 16:01 bendemboski

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).

bendemboski avatar Jan 06 '19 16:01 bendemboski

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.

mehulkar avatar Jan 07 '19 16:01 mehulkar

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: https://github.com/ember-cli/ember-exam/issues/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

brandynbennett avatar Jan 08 '19 17:01 brandynbennett

@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: https://github.com/ember-cli/ember-exam/pull/136

--

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

stefanpenner avatar Jan 08 '19 19:01 stefanpenner

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 avatar Jul 23 '22 17:07 wagenet