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

Add Ability to combine multiple feature files into one spec by Darrinholst #219

Merged
merged 4 commits into from
Aug 26, 2019

Conversation

lgandecki
Copy link
Collaborator

@lgandecki lgandecki commented Aug 24, 2019

this is a #217 PR with a bit of tweaking. The feature is described in #189

@lgandecki
Copy link
Collaborator Author

We still need documentation here, but let's get the code in first.
Maybe @laiscoolblue will be for a challenge and help us documenting this nicely :)

Copy link
Contributor

@jcundill jcundill left a comment

Choose a reason for hiding this comment

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

I'll have a closer look tomorrow - but initial thoughts are.

  1. Wow, it is a lot faster 👍

  2. I had my cypress.json file set up to just recognise .feature files as test files. This causes cypress itself to ignore any .features file you add and it keeps insisting that it can't find any specs to run. Took ages to track this down. Changed the cypress.json to

{
  "testFiles": "**/*.*(feature|features)"
}

and it was OK again.

  1. File watching seems broken - I'll need to take a closer look at this but it seemed like in cypress open if I run the All.features and then change the contents of .feature files then those changes didn't cause cypress to re-run my scenarios.

  2. non global step definitions scoped to a given feature are not working as expected - but I think you guys are already aware of this

devtools shows me something like

require('/Users/jcundill/code/cypress-cucumber-preprocessor-bug-reproduction/cypress/integration/common/index.js')
require('/Users/jcundill/code/cypress-cucumber-preprocessor-bug-reproduction/cypress/integration/common/step.js')

  
        describe(`first`, function() {
        createTestsFromFeature('First.feature', `Feature: first
  Scenario: 1
    Given I open page

  Scenario: 2
    Given I open page
`);
        })
        

        describe(`Second`, function() {
        createTestsFromFeature('Second.feature', `Feature: Second
  Scenario: 1
    Given I open page
    And I change the steps

  Scenario: 2
    When It all goes wrong
    Given I open page
`);
        })
        

At least some of those requires would need to be inside the describe blocks.

This should be fixable in the featureLoader by working out a set of requires common to all scenarios and a map of feature file name to requires specific to that feature file and requiring the locally scoped files within the describe blocks of just those files that want them.

global step definitions seem to work fine on this PR as they don't suffer from this problem.

As mentioned, will give a more detailed code level review tomorrow

@lgandecki
Copy link
Collaborator Author

Great summary @jcundill

4 - I'll have a tweak as per your suggestion in a moment. Just cleaning it up.

About 3 (watching) - I don't think this is a high priority right now, the "all" mode should really be used more for CI, than everyday development, so I'm fine with that limitation, as long as the watcher we have still works, and I don't see why that would change

.find(fileSteps => fileSteps[filePath])
[filePath].join("\n")}

createTestsFromFeature('${filePath}', \`${spec}\`);
Copy link
Contributor

Choose a reason for hiding this comment

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

think this would need to be
createTestsFromFeature('${path.basename(filePath)}', \${spec}`);
`
to recreate the way cucumber.json files are currently generated - passing in the full path causes issues on windows boxes due to the c: part.

Copy link
Collaborator Author

@lgandecki lgandecki Aug 25, 2019

Choose a reason for hiding this comment

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

hmmm ok! That leaves a possibility of feature names collisions (which I'd assumed was not a problem when every feature ran separately), but maybe that's not something to worry about prematurely

@@ -11,7 +11,7 @@ const getStepDefinitionsPaths = filePath => {
const nonGlobalPattern = `${getStepDefinitionPathsFrom(
filePath
)}/**/*.+(js|ts)`;

console.log("GOZDECKI nonGlobalPattern", nonGlobalPattern);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably need to remove this line

Copy link
Contributor

@jcundill jcundill left a comment

Choose a reason for hiding this comment

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

Looks good to me now.

@laistomazz
Copy link
Contributor

I've created a PR to update the example repository and as you can see, I had to change one step definition for it to work due to feature names collisions.

TheBrainFamily/cypress-cucumber-example#9

@lgandecki lgandecki merged commit c08fb58 into master Aug 26, 2019
@lgandecki
Copy link
Collaborator Author

🎉 This PR is included in version 1.15.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@lgandecki
Copy link
Collaborator Author

@laistomazz very nice! I think what needs to happen now also is you bumping the dependency to the 1.15.0 , then I can merge

@laistomazz
Copy link
Contributor

@lgandecki done ;)

@badeball badeball deleted the pr/217 branch April 10, 2022 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants