chore: introduce special logic for unit tests that rely on feature flags

[nija-at]

CDKv1 introduced feature flags that allowed users to opt-in to new
backwards-incompatible behaviour. There are two types of flags -
one set that need to be entirely removed in CDKv2 and their
'enabled' behaviour made the default, and another set where the
the new behaviour should be made the default, and allow users to
explicitly opt-out.

This change addresses the testing strategy for the former set (and
not the latter). There exist unit tests that test the behaviour both
when the feature flags are enabled and feature flags and disabled.
However, in v2, the feature flags will be removed and its usage
blocked. The default behaviour will be as if the feature flag is
enabled.

Introduce branching logic that treats these unit tests depending on
whether it's executed in the context of CDKv1 or CDKv2.

The logic is as follows:

• In the context of v1, all tests will execute as normal.
• In the context of v2, tests that rely on feature flag to be unset will
  not be executed.
• In the context of v2, tests that rely on feature flag to be set will
  not configure the feature flag on the App's context and continue to
  execute the test.

This logic has been captured at a single location in two methods -
testFeatureFlag() and testFeatureFlagDisabled().

To validate this logic, the unit tests that rely on the feature flags
aws-kms:defaultKeyPolicies,
aws-secretsmanager:parseOwnedSecretName and
core:enableStackNameDuplicates have been updated to use
these methods.

Forward looking: The final PR in v2 is expected to look like
this - #12644

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[rix0rrr]

Rather than complex defaulting logic and skipping tests, isn't it simpler to force all flags to a known value in the tests?

Feels like the test should be testing the functionality implied by the test, not "this works out of the box in v2" (or something).

[njlynch]

The benefit of setting this in the beforeEach() was that it was a set-and-forget it, with a reasonable default across the whole file (unless overridden). I worry with this new approach that at some point in the next few months someone -- probably me -- will write a "normal" test method, which will then break when the flags are flipped. Changing the approach to something that could more easily be run in a beforeEach() method seems safer to me.

[nija-at]

The decisions being made by these methods are - should the test be run or not, what should context property of the App be set to.

We could make this decisions in a beforeEach() but it would require each test to have checks on some local variable. Each test would look something like this -

test('...', () => {
  if (!shouldRun) { return; }
  const app = new App({ context });
});

which could also be forgotten.

Maybe something else to make this transparent?

[nija-at]

I'm also aware that someone could easily write a test that works and passes in v1 (by not using these methods and just using the standard test()) which will fail in v2 branch. I don't yet have a good strategy for this.

If we can get a model where this can be prevented, that's ideal.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 3e61f9d
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).