Skip to content

Added script to verify all tests scenario are associated with a version tag#2421

Open
SylvainSenechal wants to merge 2 commits into
development/2.15from
improvement/ZENKO-5282
Open

Added script to verify all tests scenario are associated with a version tag#2421
SylvainSenechal wants to merge 2 commits into
development/2.15from
improvement/ZENKO-5282

Conversation

@SylvainSenechal
Copy link
Copy Markdown
Contributor

Issue: ZENKO-5282

Some cleanup of a hook defined in cli-testing.

  • Adding a check to enforce that all scenarios are tagged with a version, this will be nice for documentation
  • Wanted to migrate the hook that conditionally runs tests depending on their versions tag + the version of the zenko but realize at the moment it really is useless :
    • For hotfix on older branch : the older branch has an older VERSION file AND older test files, so newer tests simply don't exist there, so TARGET_VERSION doesn't matter.
    • It could be useful if we wanted to add tests for newer versions and not run them yet (like writting a new test for 2.16.0 when we are still in 2.15.0, so that it would not be run while the version remains 2.15.0). But in practice this is not gonna be used : when we add a test, we always make sure that they work, as tests and features are pretty much shipped together

So the hook is simply removed for now.

@bert-e
Copy link
Copy Markdown
Contributor

bert-e commented May 21, 2026

Hello sylvainsenechal,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options
name description privileged authored
/after_pull_request Wait for the given pull request id to be merged before continuing with the current one.
/bypass_author_approval Bypass the pull request author's approval
/bypass_build_status Bypass the build and test status
/bypass_commit_size Bypass the check on the size of the changeset TBA
/bypass_incompatible_branch Bypass the check on the source branch prefix
/bypass_jira_check Bypass the Jira issue check
/bypass_peer_approval Bypass the pull request peers' approval
/bypass_leader_approval Bypass the pull request leaders' approval
/approve Instruct Bert-E that the author has approved the pull request. ✍️
/create_pull_requests Allow the creation of integration pull requests.
/create_integration_branches Allow the creation of integration branches.
/no_octopus Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
/unanimity Change review acceptance criteria from one reviewer at least to all reviewers
/wait Instruct Bert-E not to run until further notice.
Available commands
name description privileged
/help Print Bert-E's manual in the pull request.
/status Print Bert-E's current status in the pull request TBA
/clear Remove all comments from Bert-E from the history TBA
/retry Re-start a fresh build TBA
/build Re-start a fresh build TBA
/force_reset Delete integration branches & pull requests, and restart merge process from the beginning.
/reset Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

@SylvainSenechal SylvainSenechal requested review from a team, DarkIsDude and maeldonn May 21, 2026 12:58
Comment thread tests/functional/ctst/scripts/ensure-version-tags.ts Outdated
Comment thread tests/functional/ctst/scripts/ensureVersionTags.ts
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually no tests were breaking this rule, but wanna enforce it, it's important to know "this test was shipped with a feature on version 2.15.1"

Tested it in codespace by removing tags from some scenarios :

Image


import 'cli-testing/hooks/KeycloakSetup';
import 'cli-testing/hooks/Logger';
import 'cli-testing/hooks/versionTags';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

wrote why im doing this in the top comment of the pr

@SylvainSenechal SylvainSenechal force-pushed the improvement/ZENKO-5282 branch from e44d3eb to 9cc8b56 Compare May 21, 2026 13:23
},
"dependencies": {
"@cucumber/cucumber": "^12.7.0",
"@cucumber/cucumber": "^12.9.0",
Copy link
Copy Markdown
Contributor Author

@SylvainSenechal SylvainSenechal May 21, 2026

Choose a reason for hiding this comment

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

kinda useless because of the caret but still did it as im adding the dev dependencies

@SylvainSenechal SylvainSenechal force-pushed the improvement/ZENKO-5282 branch from 9cc8b56 to eb6b33b Compare May 21, 2026 13:30
@scality scality deleted a comment from bert-e May 21, 2026
@bert-e
Copy link
Copy Markdown
Contributor

bert-e commented May 21, 2026

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • 2 peers

Comment thread tests/functional/package.json Outdated
@SylvainSenechal SylvainSenechal force-pushed the improvement/ZENKO-5282 branch from eb6b33b to 25c5bee Compare May 21, 2026 13:41
Copy link
Copy Markdown
Contributor

@DarkIsDude DarkIsDude left a comment

Choose a reason for hiding this comment

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

Adding a check to enforce that all scenarios are tagged with a version, this will be nice for documentation for which documentation ?

https://github.com/scality/Zenko/pull/2421/changes#r3281331971 reading that. Why ? We have git history for that if needed ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We usually put them in .github/scripts ? Also we should have tests for it 🙏

}
}
}
// Background nodes are intentionally ignored
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You have twice the same comment https://github.com/scality/Zenko/pull/2421/changes#diff-637785e467dea30f38c5f19d551eefd93a7feffae2da263d44066c7d8c8c2adcR55. It's maybe better to add the why, than just describing the code (if relevant).

totalScenarios++;
const hasVersion = featureHasVersion || child.scenario.tags.some(t => VERSION_TAG_REGEX.test(t.name));
if (!hasVersion) {
offenders.push(` ${rel}:${child.scenario.location.line} — "${child.scenario.name}"`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this is needed ?

offenders.push(` ${rel}:${child.scenario.location.line} — "${child.scenario.name}"`);
}
} else if (child.rule) {
// Version tag can be inherited from Feature or Rule
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Version tag can be inherited from Feature or Rule

const hasVersion = featureHasVersion || ruleHasVersion
|| ruleChild.scenario.tags.some(t => VERSION_TAG_REGEX.test(t.name));
if (!hasVersion) {
offenders.push(` ${rel}:${ruleChild.scenario.location.line} — "${ruleChild.scenario.name}"`);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can create a new function to do that instead of duplicating the logic (ruleChild vs child).

@bert-e
Copy link
Copy Markdown
Contributor

bert-e commented May 21, 2026

Waiting for approval

The following approvals are needed before I can proceed with the merge:

  • the author

  • 2 peers

The following reviewers are expecting changes from the author, or must review again:

- name: Check for unused step definitions
working-directory: tests/functional
run: yarn unused-steps
- name: Check that all scenarios have a version tag
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we want that?

this "tagging" was introduced when CTST was used in Artesca, and embedded the tests : so there was one version of CTST and tests targeting multiple zenko versions.

the tests are now managed in git -so the information is really available via git blame or others-, I don't see the benefit of manually adding the tag on every test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, yes, there is another version of this pr where I just remove all tags + the useless before hook
I kinda liked having directly visible tags attached to the scenarios as its faster to read than commits but you're right

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