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

Support testIsolation configuration tag on Feature and Rule #1160

Merged
merged 12 commits into from
Mar 3, 2024

Conversation

mallison
Copy link
Contributor

@mallison mallison commented Feb 20, 2024

Implement #1158

Allows setting testIsolation per test suite (as it's not supported on the individual test level).

Eg.

@testIsolation(false)
Feature: non-isolated tests
  ...

will apply { testIsolation: false } to the describe block rather than each it within.

Also supported on Rule.

Feature: a feature
  @testIsolation(false)
  Rule: a rule

  Rule: another rule

@mallison mallison force-pushed the suite-level-test-configuration branch from 366b231 to f83f338 Compare February 20, 2024 10:45
@mallison
Copy link
Contributor Author

Test are failing because testIsolation was only introduced from Cypress v12 onwards.

Is there a way to skip a test based on Cypress version? (I had a quick look at the test workflow and existing tests but couldn't see any)

@badeball
Copy link
Owner

Is there a way to skip a test based on Cypress version? (I had a quick look at the test workflow and existing tests but couldn't see any)

Not as of now. You can however create a hook that will skip a test based on version, something along the lines of that shown below.

Before({ tags: "@cypress>=12" }, async function () {
  if (isPre12()) {
    return "skipped";
  }
});

.. and tag the test in question with @cypress>=12.

You can draw some inspiration off of ccf9502.

.gitignore Outdated
@@ -5,3 +5,4 @@ lib/version.ts

# Temporary directory for test execution
tmp/
node_modules
Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather that this file only contain project-specific ignores and that stuff related to your developer environment be kept in your personal gitignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Sorry about that.

Copy link
Owner

Choose a reason for hiding this comment

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

Nothing to be sorry about. I might be abnormally strict about these things, but here I am benevolent. Also, maintaining this has made me tired of sugar coating things. This PR is an excellent starting point and the test is minimal yet it verifies desired behaviour, exactly how I like it.

@@ -423,6 +434,11 @@ function createPickle(context: CompositionContext, pickle: messages.Pickle) {
const suiteOptions = tags
.filter(looksLikeOptions)
.map(tagToCypressOptions)
.filter((tag) =>
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of silently filtering this out, I think it should error similarly to how Cypress does (see your example in run-mode).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a look at this but each pickle has a list of tags, following cucumber's inheritance rules. There's no reference back to the node(s) the tag is on. Here I can't tell if @testIsolation(<value>) is on the current scenario or a parent Rule or Feature.

I could traverse the document and build a mapping of node to tag but that won't work as scenario names aren't required to be unique.

Copy link
Owner

Choose a reason for hiding this comment

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

A pickle has a astNodeIds property, which contains a reference to the scenario-node, and you can use context.astIdsMap to obtain this. This is an example of this in lib/browser-runtime.ts already.

PS: I suspect that astNodeIds contains more reference in cases of scenario outlines, so make sure to check that out and grab the correct one (and test coverage).

Copy link
Owner

Choose a reason for hiding this comment

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

Tips: you can use gherkin-javascript and jq to inspect the protocol.

$ npx gherkin-javascript --no-source tmp/features/ambiguous_keywords.feature_3/cypress/e2e/a.feature | jq .

<JSON output>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a6746ed

gherkin-javascript was very useful -- thanks!

I couldn't use the astIdsMap as a AST nodes have no parent reference so I couldn't go from tag node to the parent node. Instead I traverse the document and if the tag is defined on the current scenario node then I throw an error.

I could make this more efficient by building a map from tag id to parent node id in one pass.

I still need to look into when astNodeIds contains more than one reference -- I wanted to push this and get your thoughts before I do that.

Copy link
Owner

Choose a reason for hiding this comment

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

I couldn't use the astIdsMap as a AST nodes have no parent reference so I couldn't go from tag node to the parent node. Instead I traverse the document and if the tag is defined on the current scenario node then I throw an error.

What I meant is that you can grab the pickle's astNodeIds, use this to get a hold of the scenario node, which has information about tags that are specific to a single scenario. See below.

const scenario = assertAndReturn(
  context.astIdsMap.get(
    assertAndReturn(
      pickle.astNodeIds?.[0],
      "Expected to find at least one astNodeId"
    )
  ),
  `Expected to find scenario associated with id = ${pickle.astNodeIds?.[0]}`
);

console.log("Pickle tags", pickle.tags);
console.log("Scenario tags", (scenario as any).tags);

For the following tests, this will output a 2-element array and a 1-element array.

@foo
Feature: a feature name
  @bar
  Scenario: a scenario name
    Given a step

Copy link
Contributor Author

Choose a reason for hiding this comment

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

D'oh 🤦 I'm traversing to find the scenario ast node that I can already get from the map.

So, yeah, I can just check if the tag belongs to the corresponding ast node for the scenario -- if so throw an error. Otherwise carry on and just filter it out as it was defined on a parent Feature or Rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 5cde757

Not sure how I got myself so tied in knots with this one 😅 .

})
.reduce(Object.assign, {});

describe(feature.name || "<unamed feature>", suiteOptions, () => {
Copy link
Owner

@badeball badeball Feb 21, 2024

Choose a reason for hiding this comment

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

There's another instance where describe is invoked and that's with rules (see createRule in the same file). I think that if this were to be supported, then one ought to be able to tag rules in the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done cfe822c.

Copy link
Contributor Author

@mallison mallison Feb 22, 2024

Choose a reason for hiding this comment

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

Does configuration overriding work? Eg

@testIsolation(false)
Feature: a feature
  @testIsolation(true)
  Rule

The gherkin parser won't resolve this as tags are just strings but I guess the order of tags is being relied on here?

Copy link
Owner

Choose a reason for hiding this comment

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

I imagine that should work, yeah, and that the latest value takes precedence.

@mallison
Copy link
Contributor Author

mallison commented Feb 22, 2024

You can however create a hook that will skip a test based on version,

0f1b16c

Thanks for the pointers on this one!

@mallison mallison changed the title Apply Feature configuration tags to suite Support testIsolation configuration tag on Feature and Rule Feb 26, 2024
@mallison mallison requested a review from badeball February 28, 2024 17:02
@badeball
Copy link
Owner

Hi @mallison, thanks for your work so far. Can you verify in form of tests, that tagging a scenario outline and a example definitions, also fails appropriately?

Feature: a feature
  @testIsolation(false)
  Scenario Outline: a scenario
    Given a step
    
    Examples:
      | foo |
      | bar |
Feature: a feature
  Scenario Outline: a scenario
    Given a step
    
    @testIsolation(false)
    Examples:
      | foo |
      | bar |

@mallison
Copy link
Contributor Author

@badeball good shout on Scenario Outline and Examples. I actually didn't realise tags could be applied to Examples!

Is there a way to run Cypress in headed mode and/or add some logging to the tests? So far I've got by adding exceptions but it's a slow way to go about things 😬 .

@mallison
Copy link
Contributor Author

Can you verify in form of tests, that tagging a scenario outline and a example definitions, also fails appropriately?

Done e8893bf

@badeball
Copy link
Owner

Is there a way to run Cypress in headed mode and/or add some logging to the tests? So far I've got by adding exceptions but it's a slow way to go about things 😬 .

What I usually do in order to debug a specific test, is the following:

$ cypress open -P tmp/features/ambiguous_keywords.feature_3

.. where tmp/features/ambiguous_keywords.feature_3 corresponds to a directory containing the [Cypress test] created by the [cucumber-js test] (mouthful here).

@badeball badeball merged commit 58003f0 into badeball:master Mar 3, 2024
6 checks passed
@badeball
Copy link
Owner

badeball commented Mar 3, 2024

I've merged and released this as v20.0.2. I did some clean up in the commits following, some of that stuff was just easier to do myself than to explain.

@mallison
Copy link
Contributor Author

mallison commented Mar 4, 2024

I've merged and released this as v20.0.2

Great, thank you!

@mallison
Copy link
Contributor Author

mallison commented Mar 4, 2024

I did some clean up in the commits following, some of that stuff was just easier to do myself than to explain.

I had a look through. Looks a lot cleaner! 💅

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.

2 participants