-
Notifications
You must be signed in to change notification settings - Fork 55
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
fix: e2e tests #771
fix: e2e tests #771
Changes from 35 commits
6577ebf
63e498b
2649b52
b12306f
0fdb132
dfe3aab
5023934
12c183b
020f87a
1ca1400
307e613
7d9537b
333d33d
ab0af4e
43ec0c6
efcbca4
5fc04ca
2ab3397
d4e52ba
1c1ca48
4a1a848
15fa887
d25ba85
068c40b
6a994f0
430f0a1
f1eb76a
f7a167b
7d9bb78
8edfb56
26fe239
69aa2d4
81d285d
f45b6c8
45090b2
ac452fb
4728a4c
0d83dd0
5829bd3
ddf9e6c
e88dbee
35ad270
338457d
f98ddb9
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,2 @@ | ||
--- | ||
--- |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ name: 'poc-e2e-flow-test' | |
on: # TODO: need to change the trigger | ||
push: | ||
branches: | ||
- poc/e2e-create-amplify | ||
- poc/pms-create-amplify | ||
|
||
jobs: | ||
install: | ||
|
@@ -37,18 +37,8 @@ jobs: | |
fail-fast: false | ||
matrix: | ||
os: [ubuntu-latest, macos-latest, windows-latest] | ||
pkg-manager: [npm, yarn, pnpm] | ||
pkg-manager: [npm, yarn, yarn-stable, pnpm] | ||
node-version: [20] | ||
include: | ||
- os: ubuntu-latest | ||
pkg-manager: yarn-stable | ||
node-version: 19 # TODO: use Node 20 once https://github.com/yarnpkg/berry/pull/5961 is released | ||
- os: macos-latest | ||
pkg-manager: yarn-stable | ||
node-version: 19 # TODO: use Node 20 once https://github.com/yarnpkg/berry/pull/5961 is released | ||
- os: windows-latest | ||
pkg-manager: yarn-stable | ||
node-version: 19 # TODO: use Node 20 once https://github.com/yarnpkg/berry/pull/5961 is released | ||
env: | ||
ACKAGE_MANAGER_EXECUTABLE: ${{ matrix.pkg-manager }} # TODO: remove PACKAGE_MANAGER_EXECUTABLE once CLI is able to getPackageManager(). | ||
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. I realize it's not part of this PR, but this seems to be setting the wrong env var 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. Good catch!!! It's weird that the test ran as expected even with the wrong var name 😂 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. Oh, I see |
||
runs-on: ${{ matrix.os }} | ||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ export class AmplifyProjectCreator { | |
private readonly defaultDevPackages = [ | ||
'@aws-amplify/backend', | ||
'@aws-amplify/backend-cli', | ||
'typescript@^5.0.0', | ||
'typescript@^5.0.0', // TODO: remove this line for yarn since it's installed in amplify/ | ||
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. is there any change 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. I removed 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. checked. |
||
]; | ||
|
||
private readonly defaultProdPackages = ['aws-amplify']; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import path from 'path'; | ||
import fs from 'fs'; | ||
import _fs from 'fs/promises'; | ||
import { executeWithDebugLogger as _executeWithDebugLogger } from './execute_with_logger.js'; | ||
import { execa } from 'execa'; | ||
|
@@ -17,6 +18,12 @@ export class InitialProjectFileGenerator { | |
private readonly executeWithDebugLogger = _executeWithDebugLogger | ||
) {} | ||
|
||
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. | ||
|
||
/** | ||
* Copies the template directory to an amplify folder within the projectRoot | ||
*/ | ||
|
@@ -35,6 +42,16 @@ export class InitialProjectFileGenerator { | |
JSON.stringify(packageJsonContent, null, 2) | ||
); | ||
|
||
if (process.env.PACKAGE_MANAGER_EXECUTABLE === 'yarn-stable') { | ||
fs.writeFile(path.resolve(targetDir, 'yarn.lock'), '', (err) => { | ||
if (err) { | ||
console.error(`Error creating ${targetDir}/${targetDir}`, err); | ||
} else { | ||
console.log(`${targetDir}/yarn.lock created successfully.`); | ||
} | ||
}); | ||
} | ||
|
||
await this.initializeTsConfig(targetDir); | ||
}; | ||
|
||
|
@@ -52,6 +69,20 @@ export class InitialProjectFileGenerator { | |
'es2022', | ||
]; | ||
|
||
await this.executeWithDebugLogger(targetDir, 'npx', tscArgs, execa); | ||
if (this.executableName.startsWith('yarn')) { | ||
await this.executeWithDebugLogger( | ||
targetDir, | ||
'yarn', | ||
['add', 'typescript@^5'], | ||
execa | ||
); | ||
Comment on lines
+73
to
+78
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. why do we need this ? 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. Before we convert to hybrid PnP and Node_modules, we had to install typescript in 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. please check. 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. Just checked and we have to install typescript at both root and
|
||
} | ||
|
||
await this.executeWithDebugLogger( | ||
targetDir, | ||
this.executableName === 'npm' ? 'npx' : this.executableName, | ||
tscArgs, | ||
execa | ||
); | ||
}; | ||
} |
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.
based on the discussion from the demo, can we name these something like
yarn-classic
andyarn-modern
?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.
Renamed f98ddb9