test: Package Manager Support for deploy #869

0618 · 2024-01-05T18:52:06Z

Problem

Setup e2e test with supported Package Managers (npm, yarn-classic, yarn-modern, pnpm) for amplify deploy.

Issue number, if available:

Changes

Major change

run deployment.test.ts with PACKAGE_MANAGER_EXECUTABLE

Other changes

move test folder from the package folder ./ to a temporary folder tmp to create a fresh project
remove stop:npm-proxy
install packages for test projects

Corresponding docs PR, if applicable:

Validation

Validating in the PR check

Checklist

If this PR includes a functional change to the runtime behavior of the code, I have added or updated automated test coverage for this change.
If this PR requires a change to the Project Architecture README, I have included that update in this PR.
If this PR requires a docs update, I have linked to that docs PR above.
If this PR modifies E2E tests, makes changes to resource provisioning, or makes SDK calls, I have run the PR checks with the run-e2e label set.

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

changeset-bot · 2024-01-05T18:52:11Z

⚠️ No Changeset found

Latest commit: 20f72ce

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

0618 · 2024-01-05T19:17:13Z

packages/integration-tests/src/test-e2e/create_amplify.test.ts

@@ -115,11 +115,6 @@ void describe('create-amplify script', { concurrency: concurrency }, () => {
    );
  });

-  after(async () => {
-    // stop the npm proxy
-    await execa('npm', ['run', 'stop:npm-proxy'], { stdio: 'inherit' });


Don't stop npm-proxy because deploy.test.ts will use it.

deploy.test.ts shouldn't depend on setup from create_amplify. Each test suite should handle it's own setup and teardown. If the setup and teardown of two different test suites conflicts with each other, then we either need to run those suites serially, or figure out some other strategy for doing the setup without introducing coupling between tests.

Yea, I'm thinking if we should publish locally in the GH workflow for all the e2e tests.
Setup test for each of them would be too time-consuming and expensive.

What do you think?

I was thinking about this problem before when I was working on initial version of canary tests and I wanted to include them in normal e2e test run too. I ended up not solving this because canaries never got included in normal e2e tests.

What we need here is to develop component that manages npm-proxy lifecycle using OS primitives or file system. The reason for this that npm test runner runs each test file as separate Node process.
On the high level what that component should do is (assuming we us fs):

Attempt to start npm proxy or skip depending on state/lock recorded on file system (I'd use something like https://en.wikipedia.org/wiki/Double-checked_locking for that)

When handling test start/tear down this should somehow keep record and stop npm proxy when last test tears down.

This should be somehow resilient to inconsistent state (in case tests crash) or it should be easy to notice it and recover.

I think this piece alone deserves separate PR (which we could do on main).

One other option to solve this particular problem is to wrap node test runner cli with custom script and use programmatic api https://nodejs.org/api/test.html#runoptions . This should allow adding "before/after all test suites" logic. The advantage is that we don't have to implement cross process synchronization logic.

0618 · 2024-01-05T19:18:31Z

packages/integration-tests/src/setup_test_directory.ts


 /**
 * Root test directory.
 */
-export const rootTestDir = fileURLToPath(
-  new URL('./e2e-tests', import.meta.url)


Move the test dir from the repo to a tmpDir so that it's a fresh new project

0618 · 2024-01-05T23:06:37Z

.github/workflows/poc-e2e-flow-test.yml

      - name: Run E2E tests with ${{ matrix.pkg-manager }}
        shell: bash
        run: |
          PACKAGE_MANAGER_EXECUTABLE=${{matrix.pkg-manager}} npm run test:dir packages/integration-tests/src/test-e2e/create_amplify.test.ts
+          PACKAGE_MANAGER_EXECUTABLE=${{matrix.pkg-manager}} npm run test:dir packages/integration-tests/src/test-e2e/deployment.test.ts


not use npm run e2e at this moment because we'll add more test (e.g. sandbox.test.ts) later.

I'm assuming the end goal is to replace these with a single call to PACKAGE_MANAGER_EXECUTABLE=${{matrix.pkg-manager}} npm run e2e?

Yep. We'll eventually use PACKAGE_MANAGER_EXECUTABLE=${{matrix.pkg-manager}} npm run e2e when merge poc/package-manager-support to main

leave the create amplify as it is and add deployment test for only npm

:-)
I'd say "leave existing e2e tests as is, create new job with matrix to run create-amplify test (with added deployment step inside test)".

0618 · 2024-01-05T23:14:20Z

packages/create-amplify/src/package-manager-controller/package_manager_controller.ts

+    protected readonly executable: string,
+    protected readonly binaryRunner: string,
+    protected readonly initDefault: string[],
+    protected readonly installCommand: string,


for the comment #793 (comment) in a previous PR

edwardfoyle · 2024-01-05T23:09:49Z

.github/workflows/poc-e2e-flow-test.yml

      - name: Run E2E tests with ${{ matrix.pkg-manager }}
        shell: bash
        run: |
          PACKAGE_MANAGER_EXECUTABLE=${{matrix.pkg-manager}} npm run test:dir packages/integration-tests/src/test-e2e/create_amplify.test.ts
+          PACKAGE_MANAGER_EXECUTABLE=${{matrix.pkg-manager}} npm run test:dir packages/integration-tests/src/test-e2e/deployment.test.ts


I'm assuming the end goal is to replace these with a single call to PACKAGE_MANAGER_EXECUTABLE=${{matrix.pkg-manager}} npm run e2e?

edwardfoyle · 2024-01-05T23:11:52Z

packages/integration-tests/src/test-e2e/create_amplify.test.ts

@@ -115,11 +115,6 @@ void describe('create-amplify script', { concurrency: concurrency }, () => {
    );
  });

-  after(async () => {
-    // stop the npm proxy
-    await execa('npm', ['run', 'stop:npm-proxy'], { stdio: 'inherit' });


deploy.test.ts shouldn't depend on setup from create_amplify. Each test suite should handle it's own setup and teardown. If the setup and teardown of two different test suites conflicts with each other, then we either need to run those suites serially, or figure out some other strategy for doing the setup without introducing coupling between tests.

edwardfoyle · 2024-01-05T23:15:39Z

packages/integration-tests/src/test-project-setup/cdk/create_empty_cdk_project.ts

@@ -24,5 +25,10 @@ export const createEmptyCdkProject = async (

  await cdkCli(['init', 'app', '--language', 'typescript'], projectRoot).run();

+  await execa('npm', ['install', '-D', '@aws-amplify/auth-construct-alpha'], {


this doesn't seem like it belongs in "create_empty_cdk_project". Maybe it should be part of the createProject method in AuthTestCdkProjectCreator?

I think we can skip "raw cdk tests" in project manager expansion and just test them with npm.
The reason is that these tests are only focused on our L3 constructs and we'd be just testing CDK CLI here.

edwardfoyle · 2024-01-05T23:22:34Z

packages/integration-tests/src/test-project-setup/create_empty_amplify_project.ts

@@ -29,5 +35,24 @@ export const createEmptyAmplifyProject = async (

  await setupDirAsEsmModule(projectAmplifyDir);

+  await execa(
+    packageManagerProps?.executable ?? 'npm',


If this is going to be a repeated pattern, we may need a PackageManagerController abstraction for the test suite. It would be similar to the one in create-amplify but hopefully more lightweight to cover the installing that we need to do in tests

+1.

Other feedback is:
Do we need to "install" these or is there npm link equivalent we could use ? Or perhaps this was working because we were under single mono repo so packages were resolved automagically ?
Perhaps this is right approach but I wonder why we didn't do this before.

Btw.
If this file becomes sibling of create-amplify perhaps we should just run create-amplify and then substitute sources of generated project?

It was working before because the test directories were created in the project repo so "automagic resolution" happened. Personally I'm not opposed to keeping it that way so that we don't waste test time installing the same packages over and over

edwardfoyle · 2024-01-05T23:23:00Z

packages/integration-tests/src/test-project-setup/create_empty_amplify_project.ts

+      '@aws-amplify/backend',
+      '@aws-amplify/backend-cli',
+      '@aws-amplify/auth-construct-alpha',
+      '@aws-amplify/backend-deployer',
+      ...(projectDirName === 'data-storage-auth'
+        ? ['uuid', '@types/uuid']
+        : []),


How much, if any, of this belongs in "create_empty_amplify_project"? I can buy that installing backend and backend-cli should belong here, but not so sure about the other stuff. Perhaps individual tests should install those packages if needed.

edwardfoyle · 2024-01-05T23:24:01Z

packages/integration-tests/src/test-project-setup/get_package_manager.ts

+      };
+    default:
+      throw new Error(
+        `Unknown package manager executable: ${PACKAGE_MANAGER_EXECUTABLE}`


This is never going to hit since the assignment on line 5 assigns a default of "npm". Probably better to throw than assign a default.

sobolk

As we spoke offline but recording here too.
Please remember that deployer also uses npx to run CDK CLI and TSC.
I suggest to spike that part first and get tests passing with deployer covered somehow and then figure out how Gen 2 CLI -> deployer should communicate to pass package manager information and how abstractions there and in tests should look like.

sobolk · 2024-01-06T00:38:50Z

packages/integration-tests/src/setup_test_directory.ts

-export const rootTestDir = fileURLToPath(
-  new URL('./e2e-tests', import.meta.url)
-);
+export const rootTestDir = path.join(os.tmpdir(), 'e2e-tests');


I have to say that I like when test project appear somewhere repo root (like it was before here).
The reason is that when I work on these tests locally I can inspect the output within the same IDE instance and I don't have to jump over to OS temp dir.

Perhaps common ground could be to keep e2e-tests where they are but perhaps add some path segment per test session.
I.e. e2e-tests/<some_uuid>/<rest> ?

Btw. If we decide to follow suggestion to bootstrap test projects using create-amplify then perhaps we should make change as suggested and create folder outside of repo root.

then perhaps we should make change as suggested and create folder outside of repo root.

I'm confused. Did you suggest to create the test folder inside our outside of the repo root?

The reason why I changed it to a tmpDir was because if it's in the repo root, it shares the node_modules with the repo, which might hide some potential issues.

I realized later that if we want to "install" or use "create amplify" then it might be better to have test projects outside of repo root because of what you mentioned.

If we decide to use the repo root and share the node_modules, we should be able to remove the install commands in this PR.

Also, we may not need to use npx-proxy, so we don't have to make that change in the PR either.

I'm in favor of leaving test dirs within the repo root and not doing the extra installation bits.

on the other hand we should figure out what we want to test.
if we want to test that deployer works with "yarn resolved" workspace then perhaps going through full process makes some sense.
On the other hand it seems that we perhaps don't have to run every test project to assert that deployer works with each package manager.
So perhaps an option to consider is to have separate e2e "deployment test" that explores one deployment scenario but runs with multiple package managers.
Or maybe we should just add deployement step to create amplify tests and leave deployment tests as they are.

sobolk · 2024-01-06T00:45:10Z

packages/integration-tests/src/test-e2e/create_amplify.test.ts

@@ -115,11 +115,6 @@ void describe('create-amplify script', { concurrency: concurrency }, () => {
    );
  });

-  after(async () => {
-    // stop the npm proxy
-    await execa('npm', ['run', 'stop:npm-proxy'], { stdio: 'inherit' });


I was thinking about this problem before when I was working on initial version of canary tests and I wanted to include them in normal e2e test run too. I ended up not solving this because canaries never got included in normal e2e tests.

What we need here is to develop component that manages npm-proxy lifecycle using OS primitives or file system. The reason for this that npm test runner runs each test file as separate Node process.
On the high level what that component should do is (assuming we us fs):

Attempt to start npm proxy or skip depending on state/lock recorded on file system (I'd use something like https://en.wikipedia.org/wiki/Double-checked_locking for that)

When handling test start/tear down this should somehow keep record and stop npm proxy when last test tears down.

This should be somehow resilient to inconsistent state (in case tests crash) or it should be easy to notice it and recover.

I think this piece alone deserves separate PR (which we could do on main).

sobolk · 2024-01-06T00:46:47Z

packages/integration-tests/src/test-project-setup/cdk/create_empty_cdk_project.ts

@@ -24,5 +25,10 @@ export const createEmptyCdkProject = async (

  await cdkCli(['init', 'app', '--language', 'typescript'], projectRoot).run();

+  await execa('npm', ['install', '-D', '@aws-amplify/auth-construct-alpha'], {


I think we can skip "raw cdk tests" in project manager expansion and just test them with npm.
The reason is that these tests are only focused on our L3 constructs and we'd be just testing CDK CLI here.

sobolk · 2024-01-06T00:49:03Z

packages/integration-tests/src/test-project-setup/create_empty_amplify_project.ts

@@ -29,5 +35,24 @@ export const createEmptyAmplifyProject = async (

  await setupDirAsEsmModule(projectAmplifyDir);

+  await execa(
+    packageManagerProps?.executable ?? 'npm',


+1.

Other feedback is:
Do we need to "install" these or is there npm link equivalent we could use ? Or perhaps this was working because we were under single mono repo so packages were resolved automagically ?
Perhaps this is right approach but I wonder why we didn't do this before.

Btw.
If this file becomes sibling of create-amplify perhaps we should just run create-amplify and then substitute sources of generated project?

sobolk · 2024-01-06T00:51:02Z

packages/integration-tests/src/test-project-setup/setup_dir_as_esm_module.ts

@@ -25,6 +25,11 @@ export const setupDirAsEsmModule = async (absoluteDirPath: string) => {
    'es2022',
  ];

+  await execa('npm', ['install', 'typescript'], {


npm only ?;-)

I think this is another signal to just use create-amplify to boostrap test project.

0618 · 2024-01-08T22:20:09Z

As discussed, I'm closing the PR and create a new PR to add deployment step for in the create amplify test

0618 added 19 commits January 2, 2024 10:39

chore: change trigger

bd0ef45

chore: setup AWS credentials

247e17b

chore: enable e2e deployment

52d0f6a

chore: change test order

c4be31e

fix: change rootTestDir

2be6ae7

fix: install typescript

d3e936f

fix: install @aws-amplify/backend

67e7382

fix: install @aws-amplify/backend-cli

2e343ef

fix: install @aws-amplify/auth-construct-alpha and deployer

e83b172

chore: ignore logs

8bed4d7

fix: install uuid for data-storage-auth

339c4e5

temp: comment out cdk deploys

e1c22c0

fix: auth-cdk projects

214681d

fix: fileURLToPath error

3d15a4d

fix: fix fileURLToPath error

ba1ee8b

fix: fix fileURLToPath error again

41803e6

Merge branch 'poc/package-manager-support' into poc/pms-deploy

c1f12e6

publish locally

c52a65b

remove stop:npm-proxy

7ff5e9e

0618 commented Jan 5, 2024

View reviewed changes

0618 added 3 commits January 5, 2024 13:46

Merge branch 'poc/package-manager-support' into poc/pms-deploy

2d798e3

chore: update package.lock

436bb17

feat: test different package managers

5744032

0618 marked this pull request as ready for review January 5, 2024 23:05

0618 requested review from edwardfoyle, sobolk, Amplifiyer and srquinn21 as code owners January 5, 2024 23:05

0618 commented Jan 5, 2024

View reviewed changes

0618 added 2 commits January 5, 2024 15:09

chore: cleanup

7aadedd

chore: add readonly

20f72ce

0618 commented Jan 5, 2024

View reviewed changes

edwardfoyle reviewed Jan 5, 2024

View reviewed changes

sobolk reviewed Jan 6, 2024

View reviewed changes

0618 closed this Jan 8, 2024

0618 deleted the poc/pms-deploy branch February 14, 2024 23:33

		@@ -24,5 +25,10 @@ export const createEmptyCdkProject = async (

		await cdkCli(['init', 'app', '--language', 'typescript'], projectRoot).run();

		await execa('npm', ['install', '-D', '@aws-amplify/auth-construct-alpha'], {

test: Package Manager Support for deploy #869

test: Package Manager Support for deploy #869

Conversation

0618 commented Jan 5, 2024 • edited Loading

Problem

Changes

Validation

Checklist

changeset-bot bot commented Jan 5, 2024 • edited Loading

⚠️ No Changeset found

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolk Jan 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolk Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwardfoyle Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sobolk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0618 Jan 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0618 commented Jan 8, 2024

0618 commented Jan 5, 2024 •

edited

Loading

changeset-bot bot commented Jan 5, 2024 •

edited

Loading

sobolk Jan 6, 2024 •

edited

Loading

sobolk Jan 8, 2024 •

edited

Loading

edwardfoyle Jan 5, 2024 •

edited

Loading

sobolk left a comment •

edited

Loading

0618 Jan 7, 2024 •

edited

Loading