Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: Package Manager Support for deploy #869

Closed
wants to merge 24 commits into from
Closed

test: Package Manager Support for deploy #869

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
bd0ef45
chore: change trigger
0618 Jan 2, 2024
247e17b
chore: setup AWS credentials
0618 Jan 2, 2024
52d0f6a
chore: enable e2e deployment
0618 Jan 2, 2024
c4be31e
chore: change test order
0618 Jan 3, 2024
2be6ae7
fix: change rootTestDir
0618 Jan 3, 2024
d3e936f
fix: install typescript
0618 Jan 3, 2024
67e7382
fix: install @aws-amplify/backend
0618 Jan 3, 2024
2e343ef
fix: install @aws-amplify/backend-cli
0618 Jan 3, 2024
e83b172
fix: install @aws-amplify/auth-construct-alpha and deployer
0618 Jan 3, 2024
8bed4d7
chore: ignore logs
0618 Jan 4, 2024
339c4e5
fix: install uuid for data-storage-auth
0618 Jan 4, 2024
e1c22c0
temp: comment out cdk deploys
0618 Jan 4, 2024
214681d
fix: auth-cdk projects
0618 Jan 4, 2024
3d15a4d
fix: fileURLToPath error
0618 Jan 4, 2024
ba1ee8b
fix: fix fileURLToPath error
0618 Jan 4, 2024
41803e6
fix: fix fileURLToPath error again
0618 Jan 4, 2024
c1f12e6
Merge branch 'poc/package-manager-support' into poc/pms-deploy
0618 Jan 5, 2024
c52a65b
publish locally
0618 Jan 5, 2024
7ff5e9e
remove stop:npm-proxy
0618 Jan 5, 2024
2d798e3
Merge branch 'poc/package-manager-support' into poc/pms-deploy
0618 Jan 5, 2024
436bb17
chore: update package.lock
0618 Jan 5, 2024
5744032
feat: test different package managers
0618 Jan 5, 2024
7aadedd
chore: cleanup
0618 Jan 5, 2024
20f72ce
chore: add readonly
0618 Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .github/workflows/poc-e2e-flow-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ on: # TODO: need to change the trigger
push:
branches:
- poc/package-manager-support
- poc/pms-deploy
pull_request:
branches:
- poc/package-manager-support
Expand Down Expand Up @@ -61,7 +62,19 @@ jobs:
node-version: ${{ matrix.node-version }}
- name: Restore Build Cache
uses: ./.github/actions/restore_build_cache
- name: Configure test tooling credentials
uses: ./.github/actions/setup_profile
with:
role-to-assume: ${{ secrets.E2E_TOOLING_ROLE_ARN }}
aws-region: us-west-2
profile-name: e2e-tooling
- name: Configure test execution credentials
uses: aws-actions/configure-aws-credentials@04b98b3f9e85f563fb061be8751a0352327246b0 # version 3.0.1
with:
role-to-assume: ${{ secrets.E2E_RUNNER_ROLE_ARN }}
aws-region: us-west-2
- 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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@sobolk sobolk Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:-)
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)".

2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
*.log
logs
coverage
docs
lib
Expand Down
13 changes: 6 additions & 7 deletions packages/integration-tests/src/setup_test_directory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,19 @@
import { existsSync } from 'fs';
import fs from 'fs/promises';
import { fileURLToPath } from 'url';
import fsp from 'fs/promises';
import path from 'path';
import * as os from 'os';

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
export const rootTestDir = path.join(os.tmpdir(), 'e2e-tests');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@0618 0618 Jan 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Creates a test directory.
*/
export const createTestDirectory = async (pathName: string | URL) => {
if (!existsSync(pathName)) {
await fs.mkdir(pathName, { recursive: true });
await fsp.mkdir(pathName, { recursive: true });
}
};

Expand All @@ -23,6 +22,6 @@ export const createTestDirectory = async (pathName: string | URL) => {
*/
export const deleteTestDirectory = async (pathName: string | URL) => {
if (existsSync(pathName)) {
await fs.rm(pathName, { recursive: true, force: true });
await fsp.rm(pathName, { recursive: true, force: true });
}
};
5 changes: 0 additions & 5 deletions packages/integration-tests/src/test-e2e/create_amplify.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

  1. 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)
  2. When handling test start/tear down this should somehow keep record and stop npm proxy when last test tears down.
  3. 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).

Copy link
Member

@sobolk sobolk Jan 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

const initialStates = ['empty', 'module', 'commonjs'] as const;

initialStates.forEach((initialState) => {
Expand Down
3 changes: 2 additions & 1 deletion packages/integration-tests/src/test-e2e/deployment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { after, afterEach, before, beforeEach, describe, it } from 'node:test';
import { execa } from 'execa';
import {
createTestDirectory,
deleteTestDirectory,
Expand All @@ -8,7 +9,7 @@ import fs from 'fs/promises';
import { shortUuid } from '../short_uuid.js';
import { getTestProjectCreators } from '../test-project-setup/test_project_creator.js';
import { TestProjectBase } from '../test-project-setup/test_project_base.js';
import { userInfo } from 'os';
import { homedir, userInfo } from 'os';
import { PredicatedActionBuilder } from '../process-controller/predicated_action_queue_builder.js';
import { amplifyCli } from '../process-controller/process_controller.js';
import path from 'path';
Expand Down
6 changes: 6 additions & 0 deletions packages/integration-tests/src/test-project-setup/cdk/create_empty_cdk_project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import fsp from 'fs/promises';
import path from 'path';
import { execa } from 'execa';
import { cdkCli } from '../../process-controller/process_controller.js';
import { existsSync } from 'fs';

Expand All @@ -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'], {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stdio: 'inherit',
cwd: projectRoot,
});

return { projectName, projectRoot };
};
20 changes: 20 additions & 0 deletions packages/integration-tests/src/test-project-setup/create_empty_amplify_project.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import fs from 'fs/promises';
import path from 'path';
import { execa } from 'execa';
import { shortUuid } from '../short_uuid.js';
import { setupDirAsEsmModule } from './setup_dir_as_esm_module.js';

Expand Down Expand Up @@ -29,5 +30,24 @@ export const createEmptyAmplifyProject = async (

await setupDirAsEsmModule(projectAmplifyDir);

await execa(
'npm',
[
'install',
'-D',
'@aws-amplify/backend',
'@aws-amplify/backend-cli',
'@aws-amplify/auth-construct-alpha',
'@aws-amplify/backend-deployer',
...(projectDirName === 'data-storage-auth'
? ['uuid', '@types/uuid']
: []),
Comment on lines +43 to +49
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

],
{
stdio: 'inherit',
cwd: projectRoot,
}
);

return { projectName, projectRoot, projectAmplifyDir };
};
5 changes: 5 additions & 0 deletions packages/integration-tests/src/test-project-setup/setup_dir_as_esm_module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ export const setupDirAsEsmModule = async (absoluteDirPath: string) => {
'es2022',
];

await execa('npm', ['install', 'typescript'], {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm only ?;-)

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

stdio: 'inherit',
cwd: absoluteDirPath,
});

await execa('npx', tscArgs, {
stdio: 'inherit',
cwd: absoluteDirPath,
Expand Down
Loading