chore(cli): prevent test interference#32270
Conversation
Our CLI unit tests were interfering with each other because they were writing files from and to the current directory, which is shared between all of them. Solve it by making a non-writeable directory before running the tests, so that the tests that do that start throwing errors and we can identify them. Then fix those.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32270 +/- ##
==========================================
+ Coverage 77.17% 77.46% +0.28%
==========================================
Files 105 105
Lines 7169 7168 -1
Branches 1315 1314 -1
==========================================
+ Hits 5533 5553 +20
+ Misses 1455 1433 -22
- Partials 181 182 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
|
||
| beforeAll(() => { | ||
| tmpDir = path.join(os.tmpdir(), 'cdk-nonwritable'); | ||
| if (!fs.existsSync(tmpDir)) { |
There was a problem hiding this comment.
So If I manually change the permissions of this directory back to writable - suddenly I can locally write tests that write to this directory.
Can we make this unique per execution?
There was a problem hiding this comment.
This is not a security measure that we have to make sure nobody can circumvent.
It's a measure to help you prevent mistakes.
Also, the test will still fail on the build server (unless you put a chmod in your code, but again why are you bypassing a measure that is helping you write more robust tests? This is not for security purposes!)
There was a problem hiding this comment.
Im not trying to tighten security, i'm trying to minimize scenarios where tests pass locally but fail on the server.
A contributor gets a weird permission error -> they don't know whats going on -> so they manually "fix" the issue by changing perms on the static directory. They write tests locally, all is well. But then, as you said, it fails on the build server. Thats not a great experience. What's the downside of having a unique dir per execution?
There was a problem hiding this comment.
If someone gets as far as chmodding a directory that's called cdk-nonwritable... 🙄 I don't know how to help. I can name it cdk-nonwritable-on-purpose-do-not-chmod, but I don't know if that makes it better.
What I can do is re-chmod on every run. That doesn't require a new directory and also solves the problem where a developer will actively shoot themselves in the foot
There was a problem hiding this comment.
If they are able to get to that directory, it seems they could grep the codebase to get to this specific file and read the comment there as well.
There was a problem hiding this comment.
If there's a unique directory per execution, whatever manual permission change they do won't solve the problem.
Because they changed permission of
/tmp/unique1/cdk-nonwritable, but now the error is coming from/tmp/unique2/cdk-nowritable
And they will keep getting the error until they understand how to properly solve it. Not critical, take it or leave it.
There was a problem hiding this comment.
I did it another way
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
iliapolo
left a comment
There was a problem hiding this comment.
Remove the do-not-merge label at will.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| beforeAll(() => { | ||
| tmpDir = path.join(os.tmpdir(), 'cdk-nonwritable-on-purpose'); | ||
| fs.mkdirSync(tmpDir, { recursive: true }); | ||
| fs.chmodSync(tmpDir, 0o500); |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…nto huijbers/test-interference
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Our CLI unit tests were interfering with each other because they were writing files from and to the current directory, which is shared between all of them.
Solve it by making a non-writeable directory before running the tests, so that the tests that do that start throwing errors and we can identify them. Then fix those.
I tried papering over the issue by trying to create tempdirs automatically, but that started to introduce all kinds of errors in tests that were already doing tempdir management themselves, and it took too long to go and figure out what was wrong there, so I'm doing this instead.
Also in this PR:
silentmode for the tests. It's very annoying ifconsole.logstatements don't make it out when you're debugging.init.tsto be useless (they were there for tests), and removing those causes a lack of coverage ininit.ts's entirety, so add some tests for that file.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license