-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
(aws-ecr-assets): image-asset tests that use toBeDefined() are broken #19944
Labels
@aws-cdk/aws-ecr-assets
Related to AWS CDK Docker Image Assets
bug
This issue is a bug.
needs-triage
This issue or PR still needs to be triaged.
Comments
johns1342
added
bug
This issue is a bug.
needs-triage
This issue or PR still needs to be triaged.
labels
Apr 17, 2022
github-actions
bot
added
the
@aws-cdk/aws-ecr-assets
Related to AWS CDK Docker Image Assets
label
Apr 17, 2022
4 tasks
mergify bot
pushed a commit
that referenced
this issue
Apr 18, 2022
In my build environment, the file existence tests in the `aws-ecr-assets` module that use `.toBeDefined()` cannot fail. The [`fs.existsSync()`](https://nodejs.org/api/fs.html#fsexistssyncpath) function returns a boolean, and either `true` or `false` will count as defined for the `.toBeDefined()` test function. To test the test, remove the `packages/@aws-cdk/aws-ecr-assets/test/dockerignore-image/dockerignore` file, and it should fail due to the `foobar.txt` check: https://github.com/aws/aws-cdk/blob/4cb3b2bdb959ae398ffe2f8a5a927280f5d63306/packages/%40aws-cdk/aws-ecr-assets/test/image-asset.test.ts#L387 Fixes #19944 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
|
StevePotter
pushed a commit
to StevePotter/aws-cdk
that referenced
this issue
Apr 27, 2022
…19945) In my build environment, the file existence tests in the `aws-ecr-assets` module that use `.toBeDefined()` cannot fail. The [`fs.existsSync()`](https://nodejs.org/api/fs.html#fsexistssyncpath) function returns a boolean, and either `true` or `false` will count as defined for the `.toBeDefined()` test function. To test the test, remove the `packages/@aws-cdk/aws-ecr-assets/test/dockerignore-image/dockerignore` file, and it should fail due to the `foobar.txt` check: https://github.com/aws/aws-cdk/blob/4cb3b2bdb959ae398ffe2f8a5a927280f5d63306/packages/%40aws-cdk/aws-ecr-assets/test/image-asset.test.ts#L387 Fixes aws#19944 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
@aws-cdk/aws-ecr-assets
Related to AWS CDK Docker Image Assets
bug
This issue is a bug.
needs-triage
This issue or PR still needs to be triaged.
Describe the bug
The tests that use
toBeDefined()
cannot fail becausefs.existsSync()
returnstrue
orfalse
, and both values are defined.Expected Behavior
The dockerignore test should have failed if I removed the
.dockerignore
file.aws-cdk/packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts
Lines 374 to 390 in 4cb3b2b
Current Behavior
If you delete a
.dockerignore
file, the tests still pass.Reproduction Steps
This line should cause a test to fail, but it doesn't:
aws-cdk/packages/@aws-cdk/aws-ecr-assets/test/image-asset.test.ts
Line 387 in 4cb3b2b
Possible Solution
Change
.toBeDefined()
to.toBe(true)
.Additional Information/Context
I ran into this when testing out a PR for issue #17686 and realized that I couldn't make the test fail.
CDK CLI Version
2.20.0 (build 738ef49)
Framework Version
HEAD 4cb3b2b
Node.js Version
v14.15.4
OS
Darwin / Monterey 12.1
Language
Typescript
Language Version
TypeScript (3.9.10)
Other information
No response
The text was updated successfully, but these errors were encountered: