The-Zona-Zoo
commented
1 year ago +1 also seeing this when doing new VPC(...). Version 2.94.0 appears to be what broke it as 2.93.0 works fine.
Open trobert2 opened 1 year ago
The-Zona-Zoo
commented
1 year ago +1 also seeing this when doing new VPC(...). Version 2.94.0 appears to be what broke it as 2.93.0 works fine.
peterwoodworth
commented
1 year ago You're right, thanks for the reproduction steps. I've been able to reproduce this and see that it is breaking going from 2.93.0 to 2.94.0.
I noticed that if you remove the jest.resetModules() line, then the test succeeds.
May be related to this change https://github.com/aws/aws-cdk/pull/26854
rix0rrr
commented
1 year ago I see that there was a Jest-related change in 2.94.0 which @peterwoodworth found, but I'm 99% confident that should only affect the CDK repository itself, not any downstream users of the aws-cdk-lib library.
This is mystery. I'm looking through the set of changes and I can't even find anything else that looks suspicious 🤔
rix0rrr
commented
1 year ago To be honest I'm unsure about the purpose of resetModules inside a beforeEach() statement.
As per the documentation, it's going to affect all subsequent require() calls. In the example code you showed, some imports (which turn into require()s) have already been done at the top of the file, and then subsequent require()s that will be executed in the middle of a test will return a fresh in-memory copy of the module:
// These run at module load time, will not be affected by 'resetModules()'
import { expect as expectCDK, countResources } from '@aws-cdk/assert'
import { jest } from '@jest/globals'
import { App } from 'aws-cdk-lib'
import { DNSStack } from '../lib/stacks'
jest.useFakeTimers()
describe('testing stack', () => {
beforeEach(() => {
jest.resetModules()
// Every require() or import performed AFTER this point will get a new copy of the module
})
});What might work is doing the following:
import { expect as expectCDK, countResources } from '@aws-cdk/assert'
import { jest } from '@jest/globals'
import { App } from 'aws-cdk-lib'
import { DNSStack } from '../lib/stacks'
let expectCDK: typeof import('@aws-cdk/assert')['expect'];
let countResources: typeof import('@aws-cdk/assert')['countResources'];
let App: typeof import('aws-cdk-lib)['App'];
let DNSStack: typeof import('../lib/stacks')['DNSStack'];
beforeEach(() => {
jest.resetModules();
expectCDK = require('@aws-cdk/assert').expect;
countResources = require('@aws-cdk/assert').countResources;
App = require('aws-cdk-lib').App;
DNSStack = require('../lib/stacks').DNSStack;
});To make sure that the require() happens after resetModules is called, and symbols from different module copies aren't intermixed (they would all be from the same "load generation", if that makes sense).
rix0rrr
commented
1 year ago By the way, the code that is failing looks to be:
const constructs_1=require("constructs");
class ExportWriter extends constructs_1.Construct
^^^^ value undefined is not a constructor or nullSo it looks like constructs.Construct is undefined. Still don't understand how that would happen.
trobert2
commented
1 year ago To be honest I'm unsure about the purpose of
resetModulesinside abeforeEach()statement.As per the documentation, it's going to affect all subsequent
require()calls. In the example code you showed, someimports (which turn intorequire()s) have already been done at the top of the file, and then subsequentrequire()sthat will be executed in the middle of a test will return a fresh in-memory copy of the module:// These run at module load time, will not be affected by 'resetModules()' import { expect as expectCDK, countResources } from '@aws-cdk/assert' import { jest } from '@jest/globals' import { App } from 'aws-cdk-lib' import { DNSStack } from '../lib/stacks' jest.useFakeTimers() describe('testing stack', () => { beforeEach(() => { jest.resetModules() // Every require() or import performed AFTER this point will get a new copy of the module }) });What might work is doing the following:
import { expect as expectCDK, countResources } from '@aws-cdk/assert' import { jest } from '@jest/globals' import { App } from 'aws-cdk-lib' import { DNSStack } from '../lib/stacks' let expectCDK: typeof import('@aws-cdk/assert')['expect']; let countResources: typeof import('@aws-cdk/assert')['countResources']; let App: typeof import('aws-cdk-lib)['App']; let DNSStack: typeof import('../lib/stacks')['DNSStack']; beforeEach(() => { jest.resetModules(); expectCDK = require('@aws-cdk/assert').expect; countResources = require('@aws-cdk/assert').countResources; App = require('aws-cdk-lib').App; DNSStack = require('../lib/stacks').DNSStack; });To make sure that the
require()happens afterresetModulesis called, and symbols from different module copies aren't intermixed (they would all be from the same "load generation", if that makes sense).
Thanks for the suggestion. I don't think that would work with ESM modules.
It's worth mentioning that my package.json contains "type": "module",
So I think this results in:
ReferenceError: require is not definedI also think your import statement needs changing because:
Import declaration conflicts with local declaration of 'expectCDK'.ts(2440) so you can't have:
import { expect as expectCDK, countResources } from '@aws-cdk/assert'and
let expectCDK: typeof import('@aws-cdk/assert')['expect']
Furthermore, in order for this to work, in case you are using eslint, you would need to add:
// eslint-disable-next-line @typescript-eslint/no-var-requires@peterwoodworth, for this particular case removing resetModules indeed fixed my issue. These tests pass.
I would like to add that this solution doesn't apply to some of my other modules where I care about the context more. I would appreciate if this is still treated as a bug. What do you think?
rix0rrr
commented
1 year ago Thanks for the suggestion. I don't think that would work with ESM modules. It's worth mentioning that my package.json contains "type": "module", So I think this results in: ReferenceError: require is not defined
In that case, I think you would write:
beforeEach(async () => {
jest.resetModules();
expectCDK = (await import('@aws-cdk/assert')).expect;
countResources = (await import('@aws-cdk/assert')).countResources;
App = (await import('aws-cdk-lib')).App;
DNSStack = (await import('../lib/stacks')).DNSStack;
});I also think your import statement needs changing because:
My mistake, I meant to remove the top-level import but apparently forgot to do so. Here it is again for ESM syntax:
import { jest } from '@jest/globals'
let expectCDK: typeof import('@aws-cdk/assert')['expect'];
let countResources: typeof import('@aws-cdk/assert')['countResources'];
let App: typeof import('aws-cdk-lib)['App'];
let DNSStack: typeof import('../lib/stacks')['DNSStack'];
beforeEach(async () => {
jest.resetModules();
expectCDK = (await import('@aws-cdk/assert')).expect;
countResources = (await import('@aws-cdk/assert')).countResources;
App = (await import('aws-cdk-lib')).App;
DNSStack = (await import('../lib/stacks')).DNSStack;
});I would appreciate if this is still treated as a bug. What do you think?
I'm happy to keep this open as a bug for people to collect insights on. I have no idea what would be causing this issue though, and no idea of how to start tracking it.
It would help if you could come up with a minimal reproducing example.
The-Zona-Zoo
commented
1 year ago beforeEach(async () => { jest.resetModules(); expectCDK = (await import('@aws-cdk/assert')).expect; countResources = (await import('@aws-cdk/assert')).countResources; App = (await import('aws-cdk-lib')).App; DNSStack = (await import('../lib/stacks')).DNSStack; });
Confirming this work around does work.
To be honest I'm unsure about the purpose of
resetModulesinside abeforeEach()statement.
For some more insight, the reason we use resetModules is to test logic that uses environment variables. Obviously there are other ways to go about this but modifying the environment and then using resetModules was the easiest way based on the code.
rix0rrr
commented
1 year ago @The-Zona-Zoo, thanks, that's helpful. Are you referring to CDK code that is caching those environment variables into global variables somewhere?
Again, a reproducing example would be helpful.
The-Zona-Zoo
commented
1 year ago @rix0rrr While I can't post the full example, it's essentially something to the effect of:
let App: (typeof import('aws-cdk-lib/core'))['App'];
let Template: (typeof import('aws-cdk-lib/assertions'))['Template'];
describe('SomeCdkStack', () => {
const previousEnv = { ...process.env };
beforeEach(async () => {
if (process.env.USER) {
delete process.env.USER;
jest.resetModules();
}
App = (await import('aws-cdk-lib/core')).App;
Template = (await import('aws-cdk-lib/assertions')).Template;
});
afterEach(() => {
process.env = previousEnv;
});
});This example doesn't work with importing App and Template directly (as of 2.94.0).
The stack being tested in question has a statement similar to env.USER ?? 'someDefault' or env.USER ? 'userDefault' : 'nonUserDefault'. We use this mechanism to allow developers to have their own AWS resources with their username's appended while defaulting for the case of say beta and production environments.
Describe the bug
When upgrading from
2.88.0to2.98.0we get this error when running the same code:No code changes have been introduced. just package updates.
Expected Behavior
tests pass, deployment works
Current Behavior
Reproduction Steps
The code is redacted for obvious purposes. The code works fine with
2.88and no changes have been added to the code or the context values (see diff above, that's all that changed)Write a stack that include this:
Write a test for it
Run test:
Possible Solution
No response
Additional Information/Context
No response
CDK CLI Version
2.98.0 (build b04f852)
Framework Version
2.98.0
Node.js Version
v20.2.0
OS
mac arm
Language
Typescript
Language Version
5.2.2
Other information
No response