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
162 changes: 162 additions & 0 deletions features/suite_only_options.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
@cypress>=12
Feature: suite only options
Scenario: Configuring testIsolation on a Feature
Given additional Cypress configuration
"""
{
"e2e": {
"testIsolation": true
}
}
"""
And a file named "cypress/e2e/a.feature" with:
"""
@testIsolation(false)
Feature: a feature
Scenario: a scenario
Given a step
Scenario: another scenario
Then another step
"""
And a file named "cypress/support/step_definitions/steps.js" with:
"""
const { Given, Then } = require("@badeball/cypress-cucumber-preprocessor");
Given("a step", () => {
cy.get("body").invoke('html', 'Hello world')
});
Given("another step", () => {
cy.contains("Hello world").should("exist");
});
"""
When I run cypress
Then it passes

Scenario: Configuring testIsolation on a Rule
Given additional Cypress configuration
"""
{
"e2e": {
"testIsolation": true
}
}
"""
And a file named "cypress/e2e/a.feature" with:
"""
Feature: a feature
@testIsolation(false)
Rule: a rule
Scenario: a scenario
Given a step
Scenario: another scenario
Then another step
"""
And a file named "cypress/support/step_definitions/steps.js" with:
"""
const { Given, Then } = require("@badeball/cypress-cucumber-preprocessor");
Given("a step", () => {
cy.get("body").invoke('html', 'Hello world')
});
Given("another step", () => {
cy.contains("Hello world").should("exist");
});
"""
When I run cypress
Then it passes

Scenario: Configuring testIsolation on a Scenario fails
Given additional Cypress configuration
"""
{
"e2e": {
"testIsolation": true
}
}
"""
And a file named "cypress/e2e/a.feature" with:
"""
Feature: a feature
@testIsolation(false)
Scenario: a scenario
Given a step
"""
And a file named "cypress/support/step_definitions/steps.js" with:
"""
const { Given, Then } = require("@badeball/cypress-cucumber-preprocessor");
Given("a step", () => {
cy.get("body").invoke('html', 'Hello world')
});
"""
When I run cypress
Then it fails
And the output should contain
"""
Tag @testIsolation(false) can only be used on a Feature or a Rule
"""

Scenario: Configuring testIsolation on a Scenario Outline fails
Given additional Cypress configuration
"""
{
"e2e": {
"testIsolation": true
}
}
"""
And a file named "cypress/e2e/a.feature" with:
"""
Feature: a feature
@testIsolation(false)
Scenario Outline: a scenario
Given a step
Examples:
| foo |
| bar |
"""
And a file named "cypress/support/step_definitions/steps.js" with:
"""
const { Given, Then } = require("@badeball/cypress-cucumber-preprocessor");
Given("a step", () => {
cy.get("body").invoke('html', 'Hello world')
});
"""
When I run cypress
Then it fails
And the output should contain
"""
Tag @testIsolation(false) can only be used on a Feature or a Rule
"""

Scenario: Configuring testIsolation on Examples fails
Given additional Cypress configuration
"""
{
"e2e": {
"testIsolation": true
}
}
"""
And a file named "cypress/e2e/a.feature" with:
"""
Feature: a feature
Scenario Outline: a scenario
Given a step
@testIsolation(false)
Examples:
| foo |
| bar |
"""
And a file named "cypress/support/step_definitions/steps.js" with:
"""
const { Given, Then } = require("@badeball/cypress-cucumber-preprocessor");
Given("a step", () => {
cy.get("body").invoke('html', 'Hello world')
});
"""
When I run cypress
Then it fails
And the output should contain
"""
Tag @testIsolation(false) can only be used on a Feature or a Rule
"""
13 changes: 11 additions & 2 deletions features/support/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import path from "path";
import { promises as fs } from "fs";
import assert from "assert";
import { version as cypressVersion } from "cypress/package.json";
import { promises as fs } from "fs";
import path from "path";

export async function writeFile(filePath: string, fileContent: string) {
await fs.mkdir(path.dirname(filePath), { recursive: true });
Expand Down Expand Up @@ -106,3 +107,11 @@ export function stringToNdJson(content: string) {
export function ndJsonToString(ndjson: any) {
return ndjson.map((o: any) => JSON.stringify(o)).join("\n") + "\n";
}

export function isPost12() {
return parseInt(cypressVersion.split(".")[0], 10) >= 12;
}

export function isPre12() {
return !isPost12();
}
10 changes: 8 additions & 2 deletions features/support/hooks.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { After, Before, formatterHelpers } from "@cucumber/cucumber";
import path from "path";
import assert from "assert";
import { promises as fs } from "fs";
import { writeFile } from "./helpers";
import path from "path";
import { isPre12, writeFile } from "./helpers";

const projectPath = path.join(__dirname, "..", "..");

Expand Down Expand Up @@ -95,6 +95,12 @@ Before({ tags: "not @no-default-plugin" }, async function () {
);
});

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

After(function () {
if (
this.lastRun != null &&
Expand Down
80 changes: 74 additions & 6 deletions lib/browser-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,20 @@ import {
HOOK_FAILURE_EXPR,
INTERNAL_SPEC_PROPERTIES,
INTERNAL_SUITE_PROPERTIES,
TEST_ISOLATION_CONFIGURATION_OPTION,
} from "./constants";

import {
ITaskSpecEnvelopes,
ITaskTestCaseStarted,
ITaskTestCaseFinished,
ITaskTestStepStarted,
ITaskTestCaseStarted,
ITaskTestStepFinished,
ITaskTestStepStarted,
TASK_SPEC_ENVELOPES,
TASK_TEST_CASE_STARTED,
TASK_TEST_CASE_FINISHED,
TASK_TEST_STEP_STARTED,
TASK_TEST_CASE_STARTED,
TASK_TEST_STEP_FINISHED,
TASK_TEST_STEP_STARTED,
} from "./cypress-task-definitions";

import { notNull } from "./helpers/type-guards";
Expand Down Expand Up @@ -286,7 +287,17 @@ function createStepDescription({
}

function createFeature(context: CompositionContext, feature: messages.Feature) {
describe(feature.name || "<unamed feature>", () => {
const suiteOptions = collectTagNames(feature.tags)
.filter(looksLikeOptions)
.map(tagToCypressOptions)
.filter((tag) => {
return Object.keys(tag).every(
(key) => key === TEST_ISOLATION_CONFIGURATION_OPTION
);
})
.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.

before(function () {
beforeHandler.call(this, context);
});
Expand Down Expand Up @@ -346,7 +357,17 @@ function createRule(context: CompositionContext, rule: messages.Rule) {
}
}

describe(rule.name || "<unamed rule>", () => {
const suiteOptions = collectTagNames(rule.tags)
.filter(looksLikeOptions)
.map(tagToCypressOptions)
.filter((tag) => {
return Object.keys(tag).every(
(key) => key === TEST_ISOLATION_CONFIGURATION_OPTION
);
})
.reduce(Object.assign, {});

describe(rule.name || "<unamed rule>", suiteOptions, () => {
if (rule.children) {
for (const child of rule.children) {
if (child.scenario) {
Expand Down Expand Up @@ -420,9 +441,56 @@ function createPickle(context: CompositionContext, pickle: messages.Pickle) {
[INTERNAL_SPEC_PROPERTIES]: internalProperties,
};

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]}`
);

if ("tags" in scenario && "id" in scenario) {
const tagsDefinedOnThisScenarioTagNameAstIdMap = scenario.tags.reduce(
(acc, tag) => {
acc[tag.name] = tag.id;
return acc;
},
{} as Record<string, string>
);

if ("examples" in scenario) {
for (const example of scenario.examples) {
example.tags.forEach((tag) => {
tagsDefinedOnThisScenarioTagNameAstIdMap[tag.name] = tag.id;
});
}
}

for (const tag of pickle.tags) {
if (
looksLikeOptions(tag.name) &&
tagsDefinedOnThisScenarioTagNameAstIdMap[tag.name] === tag.astNodeId &&
Object.keys(tagToCypressOptions(tag.name)).every(
(key) => key === TEST_ISOLATION_CONFIGURATION_OPTION
)
) {
throw new Error(
`Tag ${tag.name} can only be used on a Feature or a Rule`
);
}
}
}

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 😅 .

Object.keys(tag).every(
(key) => key !== TEST_ISOLATION_CONFIGURATION_OPTION
)
)
.reduce(Object.assign, {});

if (suiteOptions.env) {
Expand Down
2 changes: 2 additions & 0 deletions lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ export const INTERNAL_SUITE_PROPERTIES = INTERNAL_PROPERTY_NAME + "_suite";

export const HOOK_FAILURE_EXPR =
/Because this error occurred during a `[^`]+` hook we are skipping all of the remaining tests\./;

export const TEST_ISOLATION_CONFIGURATION_OPTION = "testIsolation";
Loading