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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test unification + RFC232/268 support #65

Merged
merged 7 commits into from Mar 10, 2018

Conversation

Projects
None yet
4 participants
@simonihmig
Copy link
Collaborator

simonihmig commented Mar 4, 2018

This is still a WIP (see "To Do" below), so don't merge it yet! It should be fully working actually, as I have been using this branch successfully for migrating a big (mocha-based) test suite. But not yet ready for prime time... But I would certainly love some feedback asap, as so many things have changed! 馃槵

So, this needs some explanation...

What problems it solves

  • allow to use ember-cli-yadda with Ember's new testing API (RFC232 / RFC268 based)
  • adds possibility for custom test setup and teardown (fixes #47)
  • unifies previous separate test runners (acceptance, component) to a single one, allowing you to run any kind of tests: acceptance (application), component (rendering), unit etc.

Solution

  • As the new testing APIs don't use any custom moduleFor* functions anymore to create the module and setup the test, instead they only use the standard module/test or describe/ it from 'qunit' itself or mocha respectively and some setup*Test functions for test setup, this allows us to have a unified test runner, but requires some means for custom test setup.
  • For the custom test setup, I extended the runtime annotations feature, added by @sfbwalder in #52. Specifically I introduced two new hooks in yadda-annotations.js: setupFeature and setupScenario. This allows the user to provide any kind of custom test setup and teardown logic, as this module lives in user land and can be modified in any way.
  • The default yadda-annotations.js (created by the blueprints) would be extended with a default setup implementation (see https://github.com/albertjan/ember-cli-yadda/compare/master...simonihmig:test-unification?expand=1#diff-396322b0f44cb1c286e305423d02b02eR50), that adds support for 3 setup related annotations:
    • @application: will call the RFC268 setupApplicationTest() for an application tests (formerly acceptance)
    • @rendering: will call the RFC232 setupRenderingTest() for a rendering tests (component)
    • @context: will call the RFC232 setupTest() for any kind of other Ember test (unit, model)
    • a feature/scenario without any of these tags would run a plain test without any Ember specific integration
  • These default setup implementations can be changed and extended at will, e.g. to extend the @application setup with additional setup logic (see example below), or additional annotations could be implemented, e.g. a @mirage tag to call setupMirage() (required for the new testing APIs with the latest Mirage release)

Caveats

  • The unified test runner does not support any tests requiring a custom moduleFor* function, i.e. is not compatible with old school tests. So this is a breaking change! The old testing API will only work with previous releases. We probably have to maintain those for eventual bug fixes, until users have migrated their test suites to the new APIs
  • The previously available "separate steps" mode for mocha-based tests has been removed for these reasons:
    • it would not work with the unified test runner (has to wrap a scenario in describe() and each step in it(), so would require a specialized one
    • the test setup would have required some "unique" workaround, i.e. use before/after instead of beforeEach/ afterEach. But the provided setup functions do not allow this, as it would be error prone (see emberjs/ember-mocha#196). And requiring the user to implement all the setup logic with the base framework integration function from @ember/test-helpers (rather than the convenient setupApplicationTest() from qunit/mocha) would be very inconvenient, hard to maintain and also error prone (see https://github.com/emberjs/ember-mocha/blob/70cc221ac76d9a170191ab8a4ece7c853975ca49/addon-test-support/ember-mocha/setup-container-test.js#L12-L54)
    • there might be a way to achieve the desired effect (better traceability of failing steps) by some other means, maybe by some custom chai plugin to automatically prefix error messages with the step name they occured!? If not, users should probably use this.step for their custom error message, as it is used in this test suite also
  • the annotations feature (i.e. the default implementation in yadda-annotations.js) seemed to ignore any tests with an annotation, if that annotation was not present in ENV.annotations, especially when ENV.annotations was simply not used. Not sure what the exact reasoning for this was, but at least the annotations feature was focusing only on skipping tests previously, while now it is used for totally different purposes also (i.e. test setup). The previous logic was very confusing in this context, as any test tagged with e.g. @application would simply be ignored by the default checkAnnotations() implementation (https://github.com/albertjan/ember-cli-yadda/blob/master/tests/helpers/yadda-annotations.js#L29-L30). This has been changed now ( another breaking change!) as follows:
    • if ENV.annotations is used, only tests with the listed tags are run (same as before)
    • otherwise run all tests, regardless of their annotations!
    • this makes ENV.annotations effectively a "filter by tag" function (so no filtering applied if not used). I would think this is a common feature, at least I know this from other BDD frameworks (behat --tags=...)

Implementation details

  • the test runner is now a static module (in the addon-test-support tree), that it just imported from the generated test files, and not duplicated anymore for each test
  • Increased the minimum node version to 6, as it will hit end of life anyway this month IIRC, so we don't have to do another breaking change release for the node update
  • Updated this addon's test suite
    • use new testing API (e.g. use helpers from @ember/test-helpers and async/ await insteqd of andThen()
    • removed the unnecessary done() callback for sync tests
  • added some additional tests for @rendering and @context

Examples

Real world example of customized application setup + Mirage + custom test reset (clear localStorage etc.) for Mocha based tests:

...
import resetState from './reset-application-test';
import { setupApplicationTest, setupRenderingTest, setupContainerTest } from 'ember-mocha';
import setupMirage from 'ember-cli-mirage/test-support/setup-mirage';

...

function setupYaddaTest(annotations) {
  if (annotations.application) {
    return function() {
      let hooks = setupApplicationTest();
      hooks.beforeEach(resetState);
      setupMirage(hooks);
    }
  }
  ...
}

To do

  • wait for the final API for ember-mocha to be clarified (emberjs/ember-mocha#198), and ember-mocha to be released with RFC268 support
  • Update blueprints (as a separate PR probably)

@simonihmig simonihmig requested review from albertjan and sfbwalder Mar 4, 2018


function setupScenario(featureAnnotations, scenarioAnnotations) {
let setupFn = setupYaddaTest(scenarioAnnotations);
if (setupFn && (featureAnnotations.application || featureAnnotations.rendering || featureAnnotations.context)) {

This comment has been minimized.

@albertjan

albertjan Mar 5, 2018

Owner

Can we pick the most common one here? Instead of failing with an exception? Or would that cause to much headache for a new user. Maybe throw a warning, ember people have been trained to look out for those 馃槃.

This comment has been minimized.

@simonihmig

simonihmig Mar 5, 2018

Collaborator

Not sure I understand. This is guarding against having an Ember setup annotation in the feature as well as the scenario, as that would cause the setup*Test() function to be called twice (in the module/describe block of the feature and that of the scenario). This will cause some other (rather hard to understand) exceptions, something along the lines of "you cannot use the application div twice" , as there will be two beforeEach hooks called that do the same setup work (e.g. initialize application) twice.

This comment has been minimized.

@albertjan

albertjan Mar 5, 2018

Owner

Oh right I misunderstood/misread, sorry 馃憤

return new EmberPromise(function(resolve) {
yadda.Yadda(library.default(), self).yadda(scenario.steps, { ctx: {} }, function next(err, result) {
if (err) {
throw err;

This comment has been minimized.

@albertjan

albertjan Mar 5, 2018

Owner

this should be reject(err)

This comment has been minimized.

@simonihmig

simonihmig Mar 5, 2018

Collaborator

Ah, good catch! Seems I lost this when rebasing to the current master...

This comment has been minimized.

@simonihmig

simonihmig Mar 8, 2018

Collaborator

Fixed this now!

return new EmberPromise(function(resolve) {
yadda.Yadda(library.default(assert), self).yadda(scenario.steps, { ctx: {} }, function next(err, result) {
if (err) {
throw err;

This comment has been minimized.

@albertjan

albertjan Mar 5, 2018

Owner

same here

}

getDestFilePath(relativePath) {
let ext = path.extname(relativePath);
if (ext === '.feature' || ext === '.spec' || ext === '.specifiation') {
if (ext === '.feature' || ext === '.spec' || ext === '.specification') {

This comment has been minimized.

@albertjan
if (annotations.context) {
return setupTest;
}
}

This comment has been minimized.

@sfbwalder

sfbwalder Mar 5, 2018

Collaborator

Can this do setupApplicationTest by default if rendering or context are not set? I'm assuming application tests are most commonly used with BDD testing.

This comment has been minimized.

@simonihmig

simonihmig Mar 6, 2018

Collaborator

Hm, I though about this as well. But by this you would loose the ability to run plain tests without any Ember specific setup. This might be useful if you want to test some utility functions (pure functions), or some business logic classes which are not coupled to Ember and thus don't require special setup work. Like true isolated unit tests.

I certainly agree that it's mostly application tests in practice (same for us). But I think it would be nice to allow any test setup by default. Maybe we can describe this as an example in the Readme for how to customize the annotations file, e.g. how to run all tests as an application test without any required tagging in your .feature files?

@simonihmig

This comment has been minimized.

Copy link
Collaborator

simonihmig commented Mar 8, 2018

I think I have addressed all the feedback so far.

For the remaining toDos, the mocha API has been finalized, but there's not yet a release of it. However I don't think we have to wait for it to merge this. For the blueprints I would like to tackle this as a separate PR.

So from my PoV, this is ready to be merged, if you all approve this!

Before this happens, I think we should publish the current master as 0.3.0, create some kind of "legacy" branch to allow us to fix issues regarding the 0.3.x versions. Then we can merge this and any following PR for the new testing API (blueprints as mentioned) and later release this as a (breaking change) 0.4.0. As suggested here

What do you think?

@simonihmig simonihmig changed the title [WIP] Test unification + RFC232/268 support Test unification + RFC232/268 support Mar 8, 2018

@sfbwalder

This comment has been minimized.

Copy link
Collaborator

sfbwalder commented Mar 8, 2018

I like your thought to publish the backwards compatible version as 0.3.0. When we have breaking changes, I think we should go to 1.0.0 to follow semantic versioning 2.0 guidelines for breaking changes.

I'd be glad to publish current master as 0.3.0 now if that is helpful.

@simonihmig

This comment has been minimized.

Copy link
Collaborator

simonihmig commented Mar 8, 2018

I think we should go to 1.0.0 to follow semantic versioning

I think my suggestion is in line with SemVer. 1.0.0 would suggest we are committed to a stable API, which I don't know if we really are. But that's a different topic.

For 0.x.y I think we can break whenever we want to, see https://semver.org/#spec-item-4. However I think it's best practice to do such a thing in a minor release (i.e. 0.4.0, as opposed to just a patch release). With a package specifier such as ^0.3.0 in your package.json (which covers 0.3.x, but not 0.4.0), that would prevent the user from accidentally getting in the breaking change.

@sfbwalder

This comment has been minimized.

Copy link
Collaborator

sfbwalder commented Mar 8, 2018

Sure, that is fine. Ready to publish current master as 0.3.0?

@simonihmig

This comment has been minimized.

Copy link
Collaborator

simonihmig commented Mar 8, 2018

@sfbwalder I think so! 馃憤

simonihmig added some commits Feb 21, 2018

Add rendering and context annotations
Change default annotations behaviour to not exclude tests with annotations by default
Fix promise handling
Messed up during rebase...

@simonihmig simonihmig force-pushed the simonihmig:test-unification branch from cad30cc to 3788fc5 Mar 8, 2018

@sfbwalder

This comment has been minimized.

Copy link
Collaborator

sfbwalder commented Mar 8, 2018

0.3.0 is published. I upgraded one of our apps to use it and it installed and ran green.

@simonihmig

This comment has been minimized.

Copy link
Collaborator

simonihmig commented Mar 8, 2018

@sfbwalder great, thanks!

@simonihmig

This comment has been minimized.

Copy link
Collaborator

simonihmig commented Mar 8, 2018

I prepared another PR to update the blueprints, that builds on top of this one. So, @albertjan @sfbwalder ok to merge?

@lolmaus

This comment has been minimized.

Copy link
Contributor

lolmaus commented Mar 9, 2018

I've just tried this out and it works nicely for me.

This definitely needs more documentation.

This is the yadda-annotations snippet that worked for me with Mirage (note that it differs from top post):

  if (annotations.application) {
    return function (hooks) {
      setupApplicationTest(hooks)
      setupMirage(hooks)
      return hooks
    }
  }
@simonihmig

This comment has been minimized.

Copy link
Collaborator

simonihmig commented Mar 9, 2018

This definitely needs more documentation.

Absolutely! But you are using the bleeding edge here, not released code, not even merged to master yet. So you should be warned! 馃槣

@sfbwalder

This comment has been minimized.

Copy link
Collaborator

sfbwalder commented Mar 9, 2018

Minor comment... Currently the 3 default setup type annotations are application, rendering, context. These blend in with other annotations. They may be easier to reason about if they included "setup" or "test" like:

  • application-test, rendering-test, context-test

or

  • setup-application-test, setup-rendering-test, setup-context-test

Thoughts?

@simonihmig

This comment has been minimized.

Copy link
Collaborator

simonihmig commented Mar 9, 2018

Yeah, that seems reasonable. To reduce the "mental gap" even further, maybe we should name them exactly the same as the functions they actually call. Also users probably know them already when using regular tests (non-yadda). So that would be:

@sfbwalder Agree?

@sfbwalder

This comment has been minimized.

Copy link
Collaborator

sfbwalder commented Mar 10, 2018

Yes, that will be good.

@albertjan

This comment has been minimized.

Copy link
Owner

albertjan commented Mar 10, 2018

Reducing mental gap sounds good. Like the idea of lining those up.

@simonihmig

This comment has been minimized.

Copy link
Collaborator

simonihmig commented Mar 10, 2018

Ok, just renamed those as proposed!

Feel free to merge at your will! :)

}

function setupYaddaTest(annotations) {
if (annotations.setupapplicationtest) {

This comment has been minimized.

@simonihmig

simonihmig Mar 10, 2018

Collaborator

Yadda seems to convert them to lower case automatically, fyi!

@albertjan albertjan merged commit 27eee0c into albertjan:master Mar 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

This was referenced Mar 11, 2018

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