refactor test organisation by SylvainSenechal · Pull Request #2383 · scality/Zenko

SylvainSenechal · 2026-04-21T16:16:53Z

This pr is a bit savage, but it's mostly renames and changes in folder structure. Actually really not that long to review.

I recommend just pulling the branch and having a look at the /tests directory

I like this structure, I think it will be easier to maintain packages, we now have a single node 24 version to deal with accross all tests.
With this we move toward have a unique setup for all tests, this will also enable us to have utility functions that can be used across all tests which is what I need for another pr

This is one of the last pr I wanted to do before we can try to have a look (if we want to invest the time 👀 ) at re organizing the setup and installation of Zenko (all the .sh and .py files that are a bit all over the place)

EDIT FRIDAY 24TH APRIL :

Thanks for the review everyone, I apologize I re did some big force push so the comments might be all over the place
TLDR :

Won't include /workflow tests : this pr does
- Reorganise functional tests in a folder, merging cucumber and mocha tests into their own javascript project
- Moving some python setup into their own folder as they are used for both mocha and cucumber tests
- I wanna get rid of the dependabots prs and update dependencies so : Big cleanup of the requirements.txt for the python setup + many javascript package upgrade too
- Some refactoring of the node installation : everything now uses node 24

Why this pr ?
I think previous comment and diffs speak for themselves but :

with this new tests organisation we will be able to develop utility functions that can be used by any test suite.
One less package json to maintain, the more dependencies to maintain, the less we do it (old mocha test were using Vaultclient with a hash from 3 or 4 years ago, not compatible with more recent node version for example..)
Better test structure for newcomers
(probably will never do this, BUT) : Easier to bridge the gap between old mocha test and cucumber tests if we ever wanna do some migrations
New test structure will make it easier to to setup the github ci : Basically, I wanna avoid having .sh file that are like "e2e-ctst.sh, e2e-mocha.sh" : We should try to have one setup that works for everything as mush as possible to reduce complexity

bert-e · 2026-04-21T16:16:57Z

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 · 2026-04-22T15:47:08Z

end2end.yaml file was like 700 lines, with like ~6 nodes setup so I think this is better

SylvainSenechal · 2026-04-22T15:47:51Z

-        run: yarn lint
-
-  lint-ctst:
+  lint-tests:


One linter for everything now, since all tests fits in a single js project

SylvainSenechal · 2026-04-22T15:49:50Z

We had some python scripts sitting in the /zenko_tests (old mocha tests) folder.
Yet these scripts are actually used for both cucumber and mocha tests. I moved these python script to their own folder. I was even thinking about putting them in .github/something but for now they are still in /tests, just in a separate subfolder

SylvainSenechal · 2026-04-22T15:51:37Z

 5. Install node and npm.
-6. Navigate to `Zenko/tests/node_tests/backbeat`.
+6. Navigate to `Zenko/tests/mocha/backbeat`.
 7. Install node modules: `npm i`.


might need another pass later for these readmeS and .md
I did some update, but here I see for example "npm i" but we generally use yarn. I think some stuffs are stale but not gonna address in this pr

SylvainSenechal · 2026-04-22T15:52:31Z

+    'pass-on-failing-test-suite': process.env.CI_PASS_ON_TEST_FAILURE === 'true',
+    'exit': true,
+    'reporter': 'mocha-multi-reporters',
+    'reporter-options': 'configFile=mocha/mocha-reporter.json',


this used to be embedded in the package.json yarn command of the mocha test

SylvainSenechal · 2026-04-22T15:53:38Z

@@ -0,0 +1,78 @@
+import path from 'node:path';


This file, as well as the as the new package.json and tsconfig.json are really just some kind of merge of what we already had before, not much change

bert-e · 2026-04-22T15:58:06Z

Waiting for approval

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

the author
2 peers

francoisferrand · 2026-04-22T20:56:13Z

Few remarks (did not review the full PR, just had a quick look):

kind of orthogonal to your actual change, but not sure we should rename the folders (ctst → cucumber, Zenko-tests → mocha, etc) : best IMHO to name them by concept/scope than technology (which can change...), and keeping usage is simpler for people to get around (muscle memory)
I trust you that it works, but I fear mixing different frameworks (act.js, mocha/jest/cucumber...) will eventually lead to dependency hell and lots of conflicts... or require changing everything at once (think AWS Sdk migration, it was kind of convenient to be able to migrate each test suite separately)
There are more tests (recently introduced), e.g. for scripts
As far as devx, it's a mixed bag: install all deps (node packages) in a single call and run all tests from one place; but longer download and esp. build times.... And yet we will still not be able to run all tests in one command, need to run each suite separately so it is manageable...
Generally, I think it is better to have multiple "small" packages than a single "large" package (kind of distributed vs mono-repo debate). We need to share a package -which is great-, but I wonder if merging everything in really what we want (and why) or just the solution we found to allow sharing... (Btw, how did William manage to introduce a common package , in his old PR?)

maeldonn

Merging cucumber and mocha makes sense as they share most dependencies. But workflows shares nothing with them, and its postinstall (downloads the act binary) now runs for every yarn install. Could we keep it as its own package.json like before?

maeldonn · 2026-04-23T16:19:30Z

In all our tickets, documentation, and other materials, we refer to these tests as 'ctst.' Do you think it's a good idea to rename them ?

im gonna keep workflow separated, especially now that other python tests are coming inside
and also ctst instead of cucumber

benzekrimaha · 2026-04-24T11:12:27Z

+    "test:bucket_get_v2": "mocha --config mocha/.mocharc.js -t 10000 --recursive mocha/cloudserver/bucketGetV2/tests",
+    "test:bucket_policy": "mocha --config mocha/.mocharc.js -t 10000 --recursive mocha/cloudserver/bucketPolicy/tests",
+    "test:all_extensions": "run-p --aggregate-output test:crr test:aws_crr test:expiration test:transition test:ingestion_oob_s3c",
+    "postinstall": "node node_modules/@kie/act-js/scripts/postinstall.js 0.2.75"


As long as @kie/act-js lives in the root tests/package.json, every job using the new setup-node-env composite action pays the download cost — including lint-tests which has no use for it. And postinstall is silent so nobody will notice it growing.
I think that you can move workflows back into its own package (as you said), keep tests/package.json for cucumber + mocha only.

I'm re writting the pr, removed tests/workflow from this new package because yeah these tests are a bit appart from the functional tests, so we won't have this problem

benzekrimaha · 2026-04-24T11:15:40Z

+    description: GitHub App private key
+    required: true
+  repositories:
+    description: Newline-separated list of private repositories to grant access to


before the refactor, each job explicitly requested only the repos it consumes:
lint-ctst → cli-testing
end2end-2-shards-http → zenko-operator
end2end-sharded → metadata, zenko-operator
now, unless explicitly overridden (and no caller overrides), every job receives a token with access to all 3 repos. And check-workflows ends up with that token while it previously needed no private repo at all. I don't know if that is okay but rather point it as it's hidden inside this composite action.
I think we should remove the default, force each caller to pass the exact list (like the original code). If you want, keep a minimal default of cli-testing and we can document the convention clearly.

Yeah I noticed this stuff, not sure it's super important, but yeah I removed the default value and re-add explicit repo list

benzekrimaha · 2026-04-24T11:19:51Z

+    'pass-on-failing-test-suite': process.env.CI_PASS_ON_TEST_FAILURE === 'true',
+    'exit': true,


pass-on-failing-test-suite makes the process exit 0 even when tests fail.
Meaning green CI ≠ tests pass; the real verdict depends on the downstream JUnit aggregator. This is fragile: if the JUnit reporting step does not run (e.g. infra failure), the job appears green while everything broke. I know this is not new (already in the mocha package.json before), but moving it into .mocharc.js makes it the default, almost invisible to new contributors. At a minimum we need a comment explaining why.
'exit': true: Mocha explicitly does not recommend it (hides leaking handles → real bugs). Since we are touching this setup anyway, let's create a "drop --exit + fix the leaking handles" ticket and link it in the comment.

I mean as you said the pass on failing test suite was already there before, with a comment above explaining why its done, i dont really like that its a bit implicit and can be missed but not much to do here

The exit thing and the rest were also already there before but they were in the package json commands directly.

Now we have this
"test:operator": "mocha --config mocha/.mocharc.js -t 10000 mocha/init_test.js",
Before we had
"test_operator": "mocha --exit -t 10000 --reporter mocha-multi-reporters --reporter-options configFile=config.json,cmrOutput=mocha-junit-reporter+testsuitesTitle+test_operator ./init_test.js",

I don't think we should bother too much with these tests, but I can remove the args in the package.json if you think it's more visible

benzekrimaha · 2026-04-24T11:26:38Z

@@ -0,0 +1,91 @@
+{


=> mocha, mocha-junit-reporter, mocha-multi-reporters are in dependencies → these are test-only, should be devDependencies.
=> @aws-sdk/client-s3, @aws-sdk/client-iam, @aws-sdk/client-sts in devDependencies → but used at runtime by the cucumber/mocha tests. It is private: true so practically no consequence, but semantically inconsistent.
request: ^2.87.0 → deprecated since 2020.
uuid: ^3.0.1 → uuid 3 is EOL (uuid is at v11).
async: 2.1.2 → is pinned to a very old patch (v4 is current)- I don't know if this will break something-
=> werelogs: github:scality/werelogs → no tag/ref pinned. Any push on werelogs main can break the tests overnight. Pin a tag (cf. how arsenal is pinned).

Fixed in f9b3c06.

Changes made:

Moved mocha, mocha-junit-reporter, mocha-multi-reporters to devDependencies

Moved @aws-sdk/* packages to dependencies (they're used at runtime)

Updated async from 2.1.2 → ^3.2.6

Pinned werelogs to tag 8.2.3

request was already removed in a213023

benzekrimaha · 2026-04-24T11:27:37Z

+    "test:all_extensions": "run-p --aggregate-output test:crr test:aws_crr test:expiration test:transition test:ingestion_oob_s3c",
+    "postinstall": "node node_modules/@kie/act-js/scripts/postinstall.js 0.2.75"
+  },
+  "resolutions": {


Why do we need this? Usually a resolutions entry signals an upstream bad pin or a version conflict. Can we document the reason in a comment, or we fix the issue it at the source if possible?

Yes exactly there is an issue with azure client, some weird stuff with azurite moch being unbumpable because microsoft keeps on doing layoffs so their engineers are unable to update Azurite in time (Azure/Azurite#2623), making it incompatible with their latest sdk versions

Anyways, I think I have created a ticket, or at least we have this in mind, even Francois had a similar issue with Sorbet around this

benzekrimaha · 2026-04-24T11:47:47Z

+        "resolveJsonModule": true,
+        "baseUrl": "./",
+        "paths": {
+            "cli-testing": ["./node_modules/cli-testing"],


with this root tsconfig + allowJs: false + strict: true + cucumber-only paths, I am not sure how the .ts files under workflows/ get type-checked
tsc --build is in build:cucumber but without include/references it will pick up every .ts file under baseUrl, including workflows/**/*.ts
can we confirm yarn build:cucumber and yarn test:workflows pass ?

I guess no need to answer this question since I removed workflow from this folder 🌚
Which kinda raises the question about linting/building the workflow folder, but this is not the concern of this pr anymore

benzekrimaha · 2026-04-24T11:49:25Z

+        extends: [
+            ...compat.extends('scality'),
+        ],
+        rules: {


I know it is "what we had before", but by merging the configs we formally inherit all this debt in a single place. Either:

we accept this debt consciously and open a ticket to reduce it
or we use this opportunity to drop the ones that are not really relevant anymore

and for workflows: '@typescript-eslint/no-explicit-any': 'off' — since we are consolidating, can we at least plan to remove it? otherwise we lose one of the benefits of unifying...

I think for the second point no need to look at it anymore for now.

For the first point, so if I understand correctly, the best thing we want to do now is to inherit everything from scality without adding any other rules ?

Yes, inheriting from scality's base config without additional overrides is the ideal state. The current overrides exist because they were carried over from the previous separate configs during the merge.

For this PR, keeping the existing overrides is appropriate since the focus is on organizational refactoring. A follow-up ticket to reduce these eslint rule exceptions would be valuable - each override should be evaluated to see if it can be removed or if the code should be fixed to meet the base standard.

bert-e · 2026-04-24T13:29:02Z

Waiting for approval

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

the author
2 peers

SylvainSenechal · 2026-04-24T13:42:40Z

@claude[agent] hello ?

SylvainSenechal · 2026-04-24T13:43:10Z

@claude[agent] review this stuff, be VERY MEAN with me

ISSUE: ZENKO-5251

Issue: ZENKO-5251

claude · 2026-04-27T08:25:15Z

+    "scubaclient": "git+https://github.com/scality/scubaclient.git#1.1.4",
+    "uuid": "^14.0.0",
+    "vaultclient": "scality/vaultclient#8.5.6",
+    "werelogs": "github:scality/werelogs"


werelogs has no pinned tag — any push to its main branch can break CI overnight. This was flagged in the earlier review and you mentioned fixing it in f9b3c06 (pinning to 8.2.3), but the fix appears lost after the force push.

Suggested change

"werelogs": "github:scality/werelogs"

"werelogs": "github:scality/werelogs#8.2.3"

aint touching this now, i noticed many repo do the same thing

claude · 2026-04-27T08:25:16Z

+    "@google-cloud/storage": "^7.12.1",
+    "arsenal": "scality/Arsenal#8.1.38",
+    "assert": "^2.1.0",
+    "async": "2.1.2",


async is pinned to 2.1.2 (2016-era). You mentioned updating to ^3.2.6 in f9b3c06 but this also appears reverted after the force push.

Suggested change

"async": "2.1.2",

"async": "^3.2.6",

breaking changes are likely, not touching for now

SylvainSenechal force-pushed the improvement/ZENKO-5251 branch 6 times, most recently from 65f3a05 to 419f2de Compare April 22, 2026 15:41

SylvainSenechal commented Apr 22, 2026

View reviewed changes

scality deleted a comment from bert-e Apr 22, 2026

SylvainSenechal marked this pull request as ready for review April 22, 2026 15:58

SylvainSenechal requested review from a team, benzekrimaha and maeldonn April 22, 2026 15:58

maeldonn reviewed Apr 23, 2026

View reviewed changes

benzekrimaha reviewed Apr 24, 2026

View reviewed changes

SylvainSenechal force-pushed the improvement/ZENKO-5251 branch from 419f2de to b22c08f Compare April 24, 2026 13:28

SylvainSenechal requested review from benzekrimaha and maeldonn April 24, 2026 13:38

SylvainSenechal marked this pull request as draft April 24, 2026 13:41

SylvainSenechal changed the base branch from development/2.14 to improvement/ZENKO-5222 April 24, 2026 13:41

SylvainSenechal marked this pull request as ready for review April 24, 2026 13:41

Claude started work on behalf of SylvainSenechal April 24, 2026 13:42 View session

Claude finished work on behalf of SylvainSenechal April 24, 2026 13:48

Claude started work on behalf of SylvainSenechal April 24, 2026 13:48 View session

Claude finished work on behalf of SylvainSenechal April 24, 2026 13:50

SylvainSenechal force-pushed the improvement/ZENKO-5251 branch from f9b3c06 to 87dbcdc Compare April 24, 2026 14:58

scality deleted a comment from bert-e Apr 24, 2026

SylvainSenechal force-pushed the improvement/ZENKO-5251 branch from 87dbcdc to 4aeb26c Compare April 24, 2026 15:13

Base automatically changed from improvement/ZENKO-5222 to development/2.14 April 24, 2026 15:34

SylvainSenechal added 11 commits April 27, 2026 10:19

move functional tests configuration setup into their own directory

29a2981

ISSUE: ZENKO-5251

refactor cucumber and mocha tests into their own js project

ecfdb45

Issue: ZENKO-5251

update vault client to 8.5.6

ff2705e

Issue: ZENKO-5251

refactor node setup into its own action

fc09d00

Issue: ZENKO-5251

use node 24 for workflow tests

afc9984

Issue: ZENKO-5251

update python test config requirements.txt

533d441

Issue: ZENKO-5251

bump uuid to 14.0.0

9ea64cf

Issue: ZENKO-5251

bump mocha to 11.7.5

a28ae0b

Issue: ZENKO-5251

remove request from package json as its not used

8ec4a09

Issue: ZENKO-5251

remove qs useless library

b3fcf21

Issue: ZENKO-5251

move some dependencies to devDependencies

f76a226

Issue: ZENKO-5251

SylvainSenechal force-pushed the improvement/ZENKO-5251 branch from 4aeb26c to f76a226 Compare April 27, 2026 08:19

claude Bot reviewed Apr 27, 2026

View reviewed changes

		'pass-on-failing-test-suite': process.env.CI_PASS_ON_TEST_FAILURE === 'true',
		'exit': true,

	"werelogs": "github:scality/werelogs"
	"werelogs": "github:scality/werelogs#8.2.3"

Conversation

SylvainSenechal commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Apr 21, 2026

Hello sylvainsenechal,

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bert-e commented Apr 22, 2026

Waiting for approval

Uh oh!

francoisferrand commented Apr 22, 2026

Uh oh!

maeldonn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Claude AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Claude AI Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

bert-e commented Apr 24, 2026

Waiting for approval

Uh oh!

SylvainSenechal commented Apr 24, 2026

Uh oh!

SylvainSenechal commented Apr 24, 2026

Uh oh!

claude Bot Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude Bot Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal commented Apr 21, 2026 •

edited

Loading

SylvainSenechal Apr 24, 2026 •

edited

Loading