extend support for 'rule' sections in features

[makmu]

See #120

[bencompton]

Indeed, it’s true that when using “the one where” technique with scenario names summarizing the given steps, it’s not uncommon to see the same scenario names when the given steps are repeated, which would break the validation as of now. I can also see where allowing rules to be defined along with scenarios in the step definitions would be very “Jest-like”. We would probably want validation errors when rules aren’t defined, as well as code generation for rules and the scenarios within.

Rather than introducing a new defineRuleBasedFeature, what do you think about making this more of a first-class citizen by keeping only the existing defineFeature function with different call signatures? It could probably still call separate, non-exported functions behind the scenes)?

// This is for backwards compatibility and would work the same as it does today:

defineFeature(feature, (test) => {
  …
});

// This would allow both rules and rule-less scenarios, and would also support feature files with a mixture. 
// For validation, it would only enforce unique scenario names per rule, not per feature.

defineFeature(feature, ({ rule, test }) => {
  test(‘Scenario defined outside of a rule’, …);

  rule(‘Some rule’, (test) => {
    test(‘Scenario under some rule’, …);
  });
});

Here are some additional thoughts:

• It might keep things simpler if there were only one version of autoBindSteps. I can’t think of a reason to have two versions of autoBindSteps other than possibly breaking someone’s reporting. If we published this as v4, it would at least respect semver for this potentially breaking change. There would also be no need to have two sets of docs for autoBindSteps.

• Keeping the theme of API simplicity, it could make sense to deprecate the first call signature, since the new one would cover all cases. It might be possible to have the first call signature actually call the second one (see below).

• I can imagine not everyone would want to define rules in step definitions and have them validated. One way to simplify all of this might be to add a missingRuleInStepDefinitions flag to the configuration parameters to enable whether or not rules are validated. When false, calling test outside of a rule block would not throw an error, but having a two or more scenarios with the same name would still throw a validation error. So, the first call signature would end up being the same as the second one when only test is called, but not rule, and the first call signature would just set the flag to false, but call all of the same code.

• I’d also suggest using the term Rule instead of ScenarioGroup in the code to keep the domain language consistent.

Thoughts?

[makmu]

I refactored the PR to get rid of defineRuleBasedFeature and make Rule more of a first-class citizen. So now defineFeature and autoBindSteps can also be used with rules (see examples).
However, for this I had to refactor quite a bit of code and the diff has gotten rather big. I merged ParsedX types with the XFromStepDefinitions because this made validation of rules much easier for me. I also removed ScenarioGroup.
I didn't get around to implement your third suggestion (to have a missingRuleInStepDefinitions flag). So there is still the new collapseRules configuration parameter to switch rule validation on and off.

What do you think of my changes?

[bencompton]

Overall, nice work so far! Sorry, a lot going on the last couple of days—still looking over the code and will be doing some regression testing. Looks like this gets us most of the way there, with some extra refactoring and code improvements I see.

[bencompton]

Sorry for the long wait, and I thought I’d provide an update. Since this PR touches a good portion of the functionality in this library, and since there are already open issues reporting regressions from previous releases, I thought now would be a good time to take a step back and look at the testing strategy of this library. As I started writing down scenarios to manually regression test the changes in PR, I came to the realization that I was basically writing rules and scenarios, and that I might as well create an exhaustive set of feature files for this library. It also occurred to me that it should be possible to create automated integration tests for this entire library by mocking out Jest via dependency injection, which would include basically wrapping the code in feature-definition-creation.ts in a function that passes in the portions of the Jest API that are being consumed here, like describe, test, etc. Assuming it works as I imagine, this would allow mocking out Jest for integration tests, and would also open us up to potentially supporting frameworks other than Jest.

It would probably be best to defer integration tests until after this upcoming release to keep the open work in progress at a minimum. Yesterday, I got most of the feature files written with about 80 scenarios so far and a couple more feature files to go. I have pushed up a copy of your extended-rule-support branch, and should be adding the feature files today. I’ll then go through and manually test every scenario and add a comment about the outcome to every scenario. The failed tests related to this PR (including functionality that is a WIP or any regressions) would be in scope to address right away, and any other existing bugs discovered would be addressed later as part of the work of getting the integration tests to pass.

[rquast]

@bencompton I've come across the validation issue since I use the example mapping technique (Story/Rule/Example) and had a duplicate scenario title. Funnily enough, I came across the issue once I had done my initial experimental port of jest-cucumber over to web-test-runner/mocha (https://github.com/rquast/gherkin-testkit). I'll incorporate this PR into the experiment as I kind of need to work with this straight away for my other project. Have you had more time to think about cross framework compatibility?

[bencompton]

The integration-tests branch has dependency injection implemented, and I've already done a prototype with injecting in Mocha dependencies, and it worked. You essentially just replicate what the Jest implementation is now doing on that branch where you pass in test and describe from Mocha into createDefineFeature and createAutoBindSteps.

As I was regression testing this PR, there were some overall issues with the validation logic, and there were already some issues with it in master. So, I ended up creating that integration-tests branch to get integration tests all passing on master prior to pulling in this PR and adding additional integration tests and fixes. I've gotten side-tracked the last couple of weeks, but once all of this is caught up, it should be possible support other libraries. Semantically, it might be odd to consume a library called Jest Cucumber for other runners like Mocha, so I haven't determined the best way to accomplish that yet.

[rquast]

Thanks for the info. Yes, seems odd for one runner to consume something called jest-cucumber, but I guess you're also consuming @cucumber/gherkin from the cucumber runner. ;)