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

test: use GH workflow for e2e test of create amplify #636

Merged
merged 12 commits into from
Nov 13, 2023
Merged

test: use GH workflow for e2e test of create amplify #636

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
250c753
feat: setup GH workflow for create amplify
0618 Nov 10, 2023
23e320a
chore: changeset
0618 Nov 10, 2023
910c682
Merge branch 'update-package-lock' into poc/e2e-create-amplify
0618 Nov 10, 2023
fcc7e54
fix: add pkg type for modern yarn
0618 Nov 11, 2023
a7cb11b
Merge branch 'poc/package-manager-support' into poc/e2e-create-amplify
0618 Nov 11, 2023
827a51a
fix npm install
0618 Nov 13, 2023
52ba97a
feat: test Node v20 and Node v19
0618 Nov 13, 2023
6e6fe30
feat: test Node v20 and yarn-stable with Node 19
0618 Nov 13, 2023
f82dfe1
feat: exclude yarn-stable
0618 Nov 13, 2023
39b5811
feat: include node 20 and exclude node 19
0618 Nov 13, 2023
c0b304a
feat: use node 20 for others, use node 19 for yarn-stable
0618 Nov 13, 2023
065f589
chore: add TODO to use Node 20
0618 Nov 13, 2023
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
7 changes: 7 additions & 0 deletions .changeset/small-readers-check.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@aws-amplify/integration-tests': patch
'create-amplify': patch
---

- Support yarn, pnpm via env var
- Add e2e test against yarn, pnpm
82 changes: 44 additions & 38 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 @@ -5,53 +5,59 @@ name: 'poc-e2e-flow-test'
on: # TODO: need to change the trigger
push:
branches:
- poc/e2e-test
- poc/e2e-create-amplify

jobs:
create-amplify-project:
install:
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
runs-on: ${{ matrix.os }}
steps:
- name: Checkout aws-amplify/amplify-cli repo
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Setup Node.js
uses: ./.github/actions/setup_node
- name: Install or Restore Cache
uses: ./.github/actions/install_with_cache
build:
runs-on: ubuntu-latest
needs:
- install
steps:
- name: Checkout aws-amplify/amplify-cli repo
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Setup Node.js
uses: ./.github/actions/setup_node
- name: Build or Restore Build Cache
uses: ./.github/actions/build_with_cache
run_e2e_tests:
strategy:
# will finish running other test matrices even if one fails
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest, macOS-latest]
node-version: [18]
os: [ubuntu-latest, macos-latest, windows-latest]
pkg-manager: [npm, yarn, yarn-stable, pnpm]
runs-on: ${{ matrix.os }}
env:
PACKAGE_MANAGER_EXECUTABLE: ${{ matrix.pkg-manager }} # TODO: remove PACKAGE_MANAGER_EXECUTABLE once CLI is able to getPackageManager().
ACKAGE_MANAGER_EXECUTABLE: ${{ matrix.pkg-manager }} # TODO: remove PACKAGE_MANAGER_EXECUTABLE once CLI is able to getPackageManager().
runs-on: ${{ matrix.os }}
timeout-minutes: 60
needs:
- build
permissions:
# these permissions are required for the configure-aws-credentials action to get a JWT from GitHub
id-token: write
contents: read
steps:
- name: Checkout aws-amplify/amplify-cli repo
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1. TODO: try only fetch .github/workflow
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
- name: Setup Node.js
uses: actions/setup-node@8f152de45cc393bb48ce5d89d36b731f54556e65 #4.0.0
uses: actions/setup-node@5e21ff4d9bc1a8cf6de233a3057d20ec6b3fb69d # version 3.8.1
with:
node-version: ${{ matrix.node-version }}
cache: npm
- name: Local Publish create-amplify
shell: bash
run: npm run install:local && npm run build && npm run vend
- name: ${{matrix.pkg-manager}}-create-amplify-project
if: matrix.pkg-manager != 'yarn-stable'
shell: bash
run:
| # TODO: last step `create amplify` should be replaced by `run e2e`
mkdir -p /tmp/amplify-project; cd /tmp/amplify-project
npm install -g ${{matrix.pkg-manager}}
echo "$(${{matrix.pkg-manager}}) config set registry http://localhost:4873"
${{matrix.pkg-manager}} config set registry http://localhost:4873
echo "$(${{matrix.pkg-manager}}) config get registry"
${{matrix.pkg-manager}} config get registry
${{matrix.pkg-manager}} create amplify --yes

- name: yarn-stable-create-amplify-project
if: matrix.pkg-manager == 'yarn-stable'
node-version: 19
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 using ./.github/actions/setup_node because we want to use nodejs 19.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question:

Modern Yarn x Node 18/20 fails for yarnpkg/berry#4694 and yarnpkg/berry#5951 , so we use Node 19 in this PR.

However, Node 19 is already deprecated. I wonder if we should test Node 20 (exclude Modern Yarn) and Node 19 only for Modern Yarn? (We don't want to test all of them because that'd be too many combinations. e.g. 3 os x 3 package managers x 2 node version = 18)

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't test against deprecated versions of node at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modern Yarn only works with Node 19 for us 🙁 Do we want to say that we don't support Modern Yarn?

Copy link
Member

Choose a reason for hiding this comment

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

that means node version should become part of test matrix.
Can yarn run on 20 ?

Copy link
Member

Choose a reason for hiding this comment

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

That's fine but it means we need to be able [packagemanager, version] declararatively at some point to be flexible here.

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, GH workflow matrix can do that. Let me update the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need some guidance from the product team about which versions of yarn we support. 4 versions seems like 2 too many :). Also, our unofficial policy is to target support for LTS node versions and not other versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yarn 2-4 are all modern yarn, which are similar and so different from yarn 2 that we need to treat yarn 1 and yarn 2+ as 2 different versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now we just test modern yarn + Node 19. We'll move to Node 20 once yarnpkg/berry#5961 is released.

- name: Restore Build Cache
uses: ./.github/actions/restore_build_cache
- name: Run E2E tests with ${{ matrix.pkg-manager }}
shell: bash
run:
| # TODO: last step `create amplify` should be replaced by `run e2e`
mkdir -p /tmp/amplify-project; cd /tmp/amplify-project
corepack enable
echo "yarn set version stable"
yarn set version stable
echo "yarn version $(yarn --version)"
yarn config set unsafeHttpWhitelist localhost
yarn config set npmRegistryServer http://localhost:4873
PACKAGE_MANAGER_EXECUTABLE=yarn yarn create amplify --yes
run: |
PACKAGE_MANAGER_EXECUTABLE=${{matrix.pkg-manager}} npm run test:dir packages/integration-tests/src/test-e2e/create_amplify.test.ts
20 changes: 16 additions & 4 deletions packages/create-amplify/src/npm_package_manager_controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,11 @@ export class NpmPackageManagerController implements PackageManagerController {
private readonly projectRoot: string,
private readonly execa = _execa
) {}
private readonly executableName =
process.env.PACKAGE_MANAGER_EXECUTABLE || 'npm'; // TODO: replace `process.env.PACKAGE_MANAGER_EXECUTABLE` with `getPackageManagerName()` once the test infra is ready.
private readonly executableName = !process.env.PACKAGE_MANAGER_EXECUTABLE
? 'npm'
: process.env.PACKAGE_MANAGER_EXECUTABLE === 'yarn-stable'
? 'yarn'
: process.env.PACKAGE_MANAGER_EXECUTABLE; // TODO: replace `process.env.PACKAGE_MANAGER_EXECUTABLE` with `getPackageManagerName()` once the test infra is ready.

/**
* Installs the given package names as devDependencies
Expand All @@ -34,9 +37,18 @@ export class NpmPackageManagerController implements PackageManagerController {
}

try {
await executeWithDebugLogger(this.projectRoot, 'npm', args, this.execa);
await executeWithDebugLogger(
this.projectRoot,
this.executableName,
args,
this.execa
);
} catch {
throw new Error(`\`npm ${args.join(' ')}\` did not exit successfully.`);
throw new Error(
`\`${this.executableName} ${args.join(
' '
)}\` did not exit successfully.`
);
}
};
}
7 changes: 5 additions & 2 deletions packages/create-amplify/src/tsconfig_initializer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@ export class TsConfigInitializer {
private readonly existsSync = _existsSync,
private readonly execa = _execa
) {}
private readonly executableName =
process.env.PACKAGE_MANAGER_EXECUTABLE || 'npx'; // TODO: replace `process.env.PACKAGE_MANAGER_EXECUTABLE` with `getPackageManagerName()` once the test infra is ready.
private readonly executableName = !process.env.PACKAGE_MANAGER_EXECUTABLE
? 'npx'
: process.env.PACKAGE_MANAGER_EXECUTABLE === 'yarn-stable'
? 'yarn'
: process.env.PACKAGE_MANAGER_EXECUTABLE; // TODO: replace `process.env.PACKAGE_MANAGER_EXECUTABLE` with `getPackageManagerName()` once the test infra is ready.
Comment on lines +21 to +25
Copy link
Member

Choose a reason for hiding this comment

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

similar comment, here, it might be worth starting to package these things behind some abstraction. Keep env var as a mechanism, but figure out how rest of create-amplify is gonna interact with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since my next step is to get rid of env var and use a getPackageManager function, it's not necessary to refactor the env var here. Right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. On the branch, you decide when and how to take this suggestion (as well as the other one).


/**
* If tsconfig.json already exists, this is a noop. Otherwise, `npx tsc --init` is executed to create a tsconfig.json file
Expand Down
Loading