-
Notifications
You must be signed in to change notification settings - Fork 249
CLDSRV-717: Unify tests backbeat route #5907
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
CLDSRV-717: Unify tests backbeat route #5907
Conversation
Hello bourgoismickael,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
❌ 19 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR unifies test backbeat routes by refactoring backbeat replication tests to support both same-account and cross-account scenarios while adding S3C test environment support.
- Parameterizes existing replication tests to run in both same-account and cross-account scenarios
- Adds S3C environment support with appropriate test skipping logic
- Consolidates replication test execution under a unified test script
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
tests/multipleBackend/routes/routeBackbeatForReplication.js | Parameterizes replication tests for same/cross-account scenarios and adds S3C support |
tests/multipleBackend/routes/routeBackbeat.js | Adds S3C test skipping logic and updates authorization tests with proper credentials |
tests/functional/aws-node-sdk/test/multipleBackend/utils.js | Adds safety checks for missing location constraints |
tests/functional/aws-node-sdk/lib/utility/test-utils.js | Adds S3C and AWS test environment detection utilities |
package.json | Updates test script to run unified backbeat route tests |
lib/routes/routeBackbeat.js | Adds content length parsing for request monitoring |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
7902916
to
8a004c1
Compare
queryObj: { | ||
versionId: versionIdUtils.encode(testMd.versionId), | ||
}, | ||
authCredentials: backbeatAuthCredentials, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this extra parameter?
this was not required before (at least in Zenko), why do we need to pass it?
- Here, it seems the tests are about creating a new version automatically, so the caller (practically, that would be backbeat) should not provide a versionID; if we have another feature where we can create an object with a specific VersionID, that should be a separate test (if not already implemented)
- If this parameter is actually required (even for Zenko!) -even though maybe the tests did not see the issue- then we are missing some validation in the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same applies to a few other tests below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not required, I removed it.
It came from branch 7.70 where the test did not create new version but updated the same versionId
"ft_node": "cd tests/functional/raw-node && yarn test", | ||
"ft_node_routes": "cd tests/functional/raw-node && yarn run test-routes", | ||
"ft_node_route_backbeat": "cd tests/functional/raw-node && mocha --reporter mocha-multi-reporters --reporter-options configFile=$INIT_CWD/tests/reporter-config.json -t 40000 test/routes/routeBackbeat.js --exit", | ||
"ft_route_backbeat": "cd tests/multipleBackend/routes && mocha --reporter mocha-multi-reporters --reporter-options configFile=$INIT_CWD/tests/reporter-config.json -t 40000 routeBackbeat.js routeBackbeatForReplication.js --exit", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of duplicates multiple_backend_test
script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only picks backbeat route tests without veeam route and other multiple backend tests, as it was executing in ft_node_routes
before in 7.70
4d63583
to
da2cb20
Compare
416d355
to
4c73202
Compare
Because it doesn't go through arsenal's normalizeRequest. Do the same as for veeam route, to have the log field bytesReceived
This file is imported in tests used by S3C that have no external location configured
4c73202
to
0bd3d47
Compare
0bd3d47
to
4e4f7ff
Compare
/approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option
The following options are set: approve |
Build failedThe build for commit did not succeed in branch w/9.1/improvement/CLDSRV-717-unify-test-backbeat-route The following options are set: approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-717. Goodbye bourgoismickael. |
To be able to run backbeat routes test in Integration as it was before (in branch 7.70).