Skip to content

test: run e2e tests on pre-compiled packages - #23753

Merged
alan-agius4 merged 1 commit into
angular:mainfrom
jbedard:e2e-npm-deps
Sep 26, 2022
Merged

test: run e2e tests on pre-compiled packages#23753
alan-agius4 merged 1 commit into
angular:mainfrom
jbedard:e2e-npm-deps

Conversation

@jbedard

@jbedard jbedard commented Aug 16, 2022

Copy link
Copy Markdown
Contributor

The NPM packages to test must be specified via --package instead of invoking the build from within the e2e tests

This means the packages must be built before tested. This will align with bazel which will pass the bazel built (and cached) tgz files. The packages being tested could be the real npm packages before being published, although CI will probably test snapshot-like packages.

@jbedard
jbedard force-pushed the e2e-npm-deps branch 18 times, most recently from 141a25a to c7a0cb8 Compare August 17, 2022 03:26
@jbedard
jbedard marked this pull request as ready for review August 17, 2022 04:48
@jbedard
jbedard requested a review from alan-agius4 August 17, 2022 04:48
@jbedard
jbedard requested review from alan-agius4 and clydin and removed request for alan-agius4 August 17, 2022 18:54
@jbedard
jbedard force-pushed the e2e-npm-deps branch 6 times, most recently from f21bbb5 to 777cbfc Compare August 18, 2022 23:00
@jbedard
jbedard force-pushed the e2e-npm-deps branch 3 times, most recently from 8000b07 to 5edbae8 Compare August 29, 2022 18:42
@jbedard
jbedard force-pushed the e2e-npm-deps branch 3 times, most recently from e7fff1e to 30bac2c Compare September 8, 2022 17:04

@josephperrott josephperrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread tests/legacy-cli/e2e/utils/tar.ts Outdated
*/
export async function extractFile(tarball: string, filePath: string): Promise<Buffer> {
return new Promise((resolve, reject) => {
let chunks: Buffer[] | null = null;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reason that the chunks need defined in this block? Since its only referenced inside of the onentry closure, you should be able to just define it there correct?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was probably just me learning the fs/parse API. Updated 👍

@josephperrott josephperrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread tests/legacy-cli/e2e/utils/project.ts Outdated
for (const packageName of Object.keys(packages)) {
if (packageName in json['dependencies']) {
json['dependencies'][packageName] = packages[packageName].version;
json['dependencies'][packageName] = packages[packageName];

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since we have access to the version in findPackageTars can we keep using the version instead?

],
string: ['devkit', 'glob', 'ignore', 'reuse', 'ng-tag', 'tmpdir', 'ng-version'],
number: ['nb-shards', 'shard'],
array: ['package'],

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For the time being can we default this to ./dist/_*.tgz? The command itself is already a bit long and having to write --package=/dist/_*.tgz multiple times a day seems a bit unnecessary.

For context here's an example of the command now.

node tests/legacy-cli/run_e2e.js tests/legacy-cli/e2e/tests/build/library/lib-consumption-full-aot.ts --package=/dist/_*.tgz

@alan-agius4 alan-agius4 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

}
}

export const NgCLIVersion = new SemVer(packages['@angular/cli'].version);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since packages is now computed from the package tars passed on the CLI there's no guarantee that it has been put into the "global variable"s when this file is first loading, so I've moved it into a method.

Previously in this PR I had this doing require(../../../....../package.json).version instead of reading it from the loaded packages. However a) that is not the same as before this PR and b) the git repo version might not be the same as the packages being tested.

@jbedard

jbedard commented Sep 15, 2022

Copy link
Copy Markdown
Contributor Author

FWIW CODE_OF_CONDUCT.md is currently causing the CI "validation" step to fail.

@jbedard
jbedard force-pushed the e2e-npm-deps branch 2 times, most recently from f54f169 to 7b4e421 Compare September 19, 2022 18:43
@jbedard

jbedard commented Sep 19, 2022

Copy link
Copy Markdown
Contributor Author

Updated and squashed, see diff

@alan-agius4 alan-agius4 added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed action: merge The PR is ready for merge by the caretaker labels Sep 23, 2022
Comment thread docs/DEVELOPER.md Outdated
@@ -84,6 +84,7 @@ You can find more info about debugging [tests with Bazel in the docs.](https://g

- Run: `node tests/legacy-cli/run_e2e.js`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you add a line here that now running yarn build needs to be ran prior to node tests/legacy-cli/run_e2e.js as the latter will not build the packages.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated

The NPM packages being tested must be pre-compiled and the tar packages specified via --package. This way the real packages such as snapshots, release artifacts or cached packages can be tested. Previously the e2e tests compiled and packaged during test execution.
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Sep 26, 2022
@alan-agius4
alan-agius4 merged commit 2624d89 into angular:main Sep 26, 2022
@angular-automatic-lock-bot

Copy link
Copy Markdown

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Oct 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants