-
Notifications
You must be signed in to change notification settings - Fork 61
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.
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
feat: init e2e test workflow for Package Manager Support #533
Changes from 50 commits
2f4e039
04e6ee4
d88786b
c0eb0d4
215acbf
95842bb
aaebae2
45d990c
ee1c6c3
f856bb8
1b11e6e
fd937e1
6e6d06f
41c89ea
c690620
fd2d010
f030ce0
974bfa6
a938e15
c71302f
71af9a8
b28d8b9
b41bb05
e722f11
7dca6da
70fad76
dc2df4b
c423170
518a43c
7b88772
a085162
988637e
be3a456
fb7fa3f
3d6d301
2b90bcc
4d3ebc3
7ca0144
347dae0
c0b1bde
16fdea3
d3dc3ec
3f0f5b2
c3b7998
a574541
73ff607
00b9499
fbf5b1a
9c6d8a6
185be00
a59cf5a
3aba42f
633ec61
863dc24
d0119b2
47d2a4f
3de87ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
'create-amplify': patch | ||
--- | ||
|
||
1. Create Amplify (temporarily) uses environment variable for Package Manager | ||
2. Create Amplify uses `yes` option to choose the default value for prompts |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
# Description: This workflow mimic the flow of behaviors when user using Amplify CLI; | ||
|
||
name: 'poc-e2e-flow-test' | ||
|
||
on: # TODO: need to change the trigger | ||
push: | ||
branches: | ||
- poc/e2e-test | ||
|
||
jobs: | ||
create-amplify-project: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should figure out how to change this from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if it's a workflow, we can make is a re-usable GH workflow, however, it seems like just a shared workflow setup (metrics, etc), which I think we have to write it each time |
||
strategy: | ||
fail-fast: false | ||
matrix: | ||
os: [ubuntu-latest, windows-latest, macOS-latest] | ||
node-version: [18] | ||
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(). | ||
steps: | ||
- name: Checkout aws-amplify/amplify-cli repo | ||
uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1. TODO: try only fetch .github/workflow | ||
- name: Setup Node.js | ||
uses: actions/setup-node@8f152de45cc393bb48ce5d89d36b731f54556e65 #4.0.0 | ||
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: | | ||
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' | ||
shell: bash | ||
run: | | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The actions should focus on setting up the runner machine. I.e. making sure package manager is installed and in desired version and available in $PATH (this is a candidate for custom action). But the test scenario itself should be placed in TS files that contain existing e2e tests. I.e. tests and test framework should be changed to accept package manager setting somehow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you mean after the GH workflow setup the test environments, and we should run packages/integration-tests/src/test-e2e/create_amplify.test.ts ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and later just all e2e tests when they work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Should I update in this PR to run the test-e2e/create_amplify.test.ts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes lets do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. test-e2e/create_amplify.test.ts takes more effort to modify than I anticipated. Do you think we can get this PR merged first? It would make it smaller to review as well :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you want to merge this please leave todo comment in workflow that summarizes our discussion here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. Done 3de87ad |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,8 @@ export class NpmPackageManagerController implements PackageManagerController { | |
private readonly projectRoot: string, | ||
private readonly execa = _execa | ||
) {} | ||
private readonly executableName = 'npm'; | ||
private readonly executableName = | ||
process.env.PACKAGE_MANAGER_EXECUTABLE || 'npm'; // TODO: replace `process.env.PACKAGE_MANAGER_EXECUTABLE` with `getPackageManagerName()` once the test infra is ready. | ||
|
||
/** | ||
* Installs the given package names as devDependencies | ||
|
@@ -24,9 +25,11 @@ export class NpmPackageManagerController implements PackageManagerController { | |
packageNames: string[], | ||
type: DependencyType | ||
): Promise<void> => { | ||
const args = ['install'].concat(...packageNames); | ||
const args = [this.executableName === 'yarn' ? 'add' : 'install'].concat( | ||
...packageNames | ||
); | ||
if (type === 'dev') { | ||
args.push('--save-dev'); | ||
args.push('-D'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code here is already merged in https://github.com/aws-amplify/samsara-cli/pull/507/files, I'm not sure why it shows up in this PR... |
||
} | ||
await this.execa(this.executableName, args, { | ||
stdio: 'inherit', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we'll need to hook this up into health_checks.
Which means either stuffing this into health_checks OR investing in reusable workflow. See https://docs.github.com/en/actions/using-workflows/reusing-workflows (we did this for canaries in classic CLI).
@edwardfoyle what do you think? health_checks.yml started to be quite big.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add it to health_checks once it's merged to
main