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

Runtime annotations #52

Merged
merged 7 commits into from
Feb 17, 2018
Merged

Conversation

benwalder
Copy link
Collaborator

This is a first cut at allowing the use of annotations to specify which tests to run at run-time as was considered in #17. The code needs some clean-up, so consider this an initial proposal that is very open to everyone's input.

The gist is this:

  1. Put an annotation (e.g. @smoke) on a feature or scenario in the .feature file.
  2. Specify which annotations to run in config/environment.js in the appropriate environment section. This could be made more flexible by passing in an arg on ember t or using environment variables to specify the annotations to run.
  3. The acceptance test runner calls a user defined module (e.g. yadda-run-test-helper.js), passing it the annotations on the feature or scenario. The user defined module has the logic to compare the annotations on the test to the requested annotations in the configuration and returns true/false. If true, the acceptance test runner runs the test; if false it does not run the test.
  4. The user defined module is configurable in ember-cli-build.js. This configuration is used by the feature parser to import the module if it exists.
  • Using a user defined module keeps the test runner from enforcing rules on how annotations are used.

Please provide opinions and I will make updates to the code and this pull request as needed.

@albertjan
Copy link
Collaborator

Very cool! Thanks so much for this. I'm not entirely clear on how this would work for the user, but that will get clearer with more study-ing. I have one question now:

Do I understand correctly that if people want to handle custom annotations they'll need to create a yadda-run-test-helper.js file? If so it might be nice to add this file to the default blueprint, so it's created with the skip annotation by default so people can see how it works.

@benwalder
Copy link
Collaborator Author

Do I understand correctly that if people want to handle custom annotations they'll need to create a yadda-run-test-helper.js file?

Yes. They can name it what they want and put what ever logic in it they want. If we want to provide a default implementation we could provide that as a blueprint.

@albertjan
Copy link
Collaborator

I think it would make sense to handle all annotations the same way.

@benwalder
Copy link
Collaborator Author

I simplified some things:

  1. Annotations are all (including @ignore) handled in tests/helpers/yadda-annotations.js.
    • The configuration in ember-cli-build.js has been removed. The feature parser always imports tests/helpers/yadda-annotations.
    • See acceptance.js for how it is called.
  2. A default implementation of yadda-annotations.js is in the blueprint and will be put in the consuming app's tests/helpers at install time.
  3. The default implementation provides backwards compatibility. See comments in yadda-annotations.js for more details.
  4. If the consumer needs something other than than the default implementation, they can change it in tests/helpers/yadda-annotations.js.

I didn't know how to change the ember's ENV.annotations between running feature acceptance tests.
Currently config/environment.js needs to be changed manually and the tests rerun to test all annotation scenarios. See comments in config/environment.js for more info. Thoughts on how to automate this?

Does any of this break mocha consumers?

Let me know your thoughts on where this is going, and I will continue to adjust and clean it up.

// if a runFeature function is provided, use it to determine if should run the feature
if (typeof(runFeature) === 'function' && !runFeature(feature.annotations)) {
console.log('not running feature: ', feature.title);
let runFeature = yaddaAnnotations.runFeature(feature.annotations);
Copy link
Collaborator

@albertjan albertjan Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if runFeature would return a function and if we'd call that function from the current scope would we be able to access all the stuff in the current scope?

i.e.:

if (annotations.ignore) {
  return function() {
    skip(`Feature ${feature.title}`, function (assert) {});
  }
}

This way you could do anything to the test, not just skip it. I'm not sure if we could teach this to the user though. We definitely should NOT rely on them having to do this stuff:

      test('Scenario: ${scenario.title}', function(assert) {
        let self = this;
        return new Ember.RSVP.Promise(function(resolve) {
          yadda.Yadda(library.default(assert), self).yadda(scenario.steps, { ctx: {} }, function next(err, result) {
            if (err) {
              throw err;
            }
            resolve(result);
          });
        });

I don't know how that would work exactly. We will definitely have to change the mocha runner to do this annotations dance too though.

@benwalder
Copy link
Collaborator Author

benwalder commented Oct 3, 2017

Is this anywhere close to the concept you are thinking?

The code to run the tests is still in the control of the addon. With the way it is coded, there would need to be a decision on what params to pass to the functions. We don't have a use case to do anything more than bypass the test. Do you have a use case in mind?

@benwalder
Copy link
Collaborator Author

Thoughts on where this is going? I'll clean it up and add some doc if so.

@albertjan
Copy link
Collaborator

Yeah definitely, I'll try to have a look this evening. I think this is a very worth while extension. We'll have to look into mocha support and upgrade path, from the top of my head. I'll look into this asap. 😄

@albertjan
Copy link
Collaborator

I looked at it yesterday and got kind of lost in getting ember try to install ember-cli-mocha to test the mocha stuff and then my npm / node install disintegrated. 👍

I really like what you've done here and I think it's the correct way to handle annotations. Giving sensible defaults and providing extensibility for future use.

@simonihmig and @adamjmcgrath and @mschinis If you guys have time and are still using ember-cli-yadda do you want to read through this and see what you think 😄

@adamjmcgrath
Copy link
Contributor

This branch is failing for mocha because of #53 if you rebase it against master I'll take a look

@mschinis
Copy link
Contributor

hey @albertjan, we're currently not using ember-cli-yadda 😊

@benwalder
Copy link
Collaborator Author

benwalder commented Dec 26, 2017

I'm not sure I re-based this correctly. I think it is now re-based.

@adamjmcgrath can you try this out for mocha?

@adamjmcgrath
Copy link
Contributor

$ npm install -g [email protected]
$ ember new yaddatest
$ cd yaddatest/
$ ember install ember-cli-mocha
$ ember install StateFarmIns/ember-cli-yadda#runtime-annotations
$ ember g ember-cli-yadda
$ ember g feature make-feature
$ ember t
...
not ok 1 Chrome 63.0 - TestLoader Failures yaddatest/tests/acceptance/make-feature-test: could not be loaded
    ---
        message: >
            Could not find module `yaddatest/tests/helpers/yadda-annotations` imported from `yaddatest/tests/acceptance/make-feature-test`

I guess you need a mocha equivalent to blueprints/qunit/ember-cli-yadda/files/tests/helpers/yadda-annotations.js

@benwalder
Copy link
Collaborator Author

@adamjmcgrath I added yadda-annotations for mocha, then followed the steps you did and it worked. I then did some additional manual testing to

  • ensure it was backwards compatible with @ignore.
  • included / excluded features and scenarios for other annotations

It seem to be working. Can you please try again?

@adamjmcgrath
Copy link
Contributor

tests/helpers/yadda-annotations.js
  47:3  error  Unexpected console statement  no-console

Other than that, LGTM

@benwalder
Copy link
Collaborator Author

Tests are failing when attempting to run with ember 3.0. I think this add-on needs updated to 2.x. Should I open an issue and do a PR?

@albertjan
Copy link
Collaborator

@eltjo-k Hey man! How are you doing? Can you give these guys write access to this repo? I don't have the time to maintain it at the moment.

@albertjan
Copy link
Collaborator

Alright guys I made you both contributors. if you can decide amongst yourselves that it works merge at will 👍 let me know when I should do a npm release. 😄

@benwalder
Copy link
Collaborator Author

@albertjan @adamjmcgrath I think this is ready to merge. Any objections?

@albertjan
Copy link
Collaborator

albertjan commented Feb 17, 2018 via email

@benwalder benwalder changed the title WIP: Runtime annotations Runtime annotations Feb 17, 2018
@benwalder benwalder merged commit cbb1c64 into kaliber5:master Feb 17, 2018
@benwalder benwalder deleted the runtime-annotations branch February 17, 2018 20:36
@benwalder
Copy link
Collaborator Author

@albertjan ready for new release, thx

@albertjan
Copy link
Collaborator

@sfbwalder do you have a npm account?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants