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.

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

run e2e tests in isolated child processes #23245

Merged
merged 3 commits into from
Jun 7, 2022
Merged

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented May 28, 2022

Note I'd like to merge #23130 first. DONE

This is to stop mixing the angular-cli test-runner environment with the cli environments being tested. Essentially launching each test in a subprocess to a) not mix with the test-runner PATH, npm modules etc, b) prevent environment (vars, cwd, ...) side effects from one test effecting the next tests or the test-runner.

Prefactor commits (can be merged on their own ahead of time instead of squashing):

  • Wait for killAllProcesses to actually complete before proceeding. Killing a procedure is async but was not being await-ed. This creates race conditions but once tests are run in a subprocess those are far more likely (the subprocess dies before killing subprocesses completes). EDIT: was done in f70557a
  • Refactor the main e2e_runner.ts into separate methods, async/await instead of .then.
  • Distinguish test setup vs tests better (in preparation for tests being launched in subprocesses, also distinguishes setup vs test better in the stdout).

Main change:

  • A specific version of npm + yarn are installed alongside the angular-cli npm package being tested. This will be required in bazel where nothing outside the sandbox (global node/npm/yarn) is accessible. This also sets specific version numbers which could be dynamic in the future.
  • Tests run in a sandboxed subprocess environment using those specific npm + yarn + cli packages.
  • Tests no longer have access to the node_modules or environment of the test-runner, only the modules installed within that sandboxed environment. So things like puppeteer now need to be installed within the sandboxed test environment (previously it was using the one in the git repo node_modules). The protractor used in the tests is now the correct one in the test project node_modules (not the one the cli git repo node_modules), so webdriver-manager must now run on that test project and not only the git repo.

Overview:

  • launch "setup" in the main process to setup the test environment (no real changes from before)
  • launch each "initializer" in subprocesses (some of the previous "setup" scripts are now "initializers" which run in the subprocess, but no big changes)
  • launch each test in the subprocesses

@jbedard jbedard force-pushed the e2e-local-npm branch 7 times, most recently from 790a1a9 to bd671ea Compare May 30, 2022 09:51
for (const [initializerIndex, initializer] of allInitializers.entries()) {
printSetupHeader(initializer, initializerIndex, allInitializers.length, 'initializer');

await runInitializer((lastTestRun = initializer));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comment that runInitializer runs ng init and creates test-project

@jbedard jbedard force-pushed the e2e-local-npm branch 4 times, most recently from b527198 to 4558799 Compare May 31, 2022 01:37
@jbedard jbedard marked this pull request as ready for review May 31, 2022 01:37
@jbedard jbedard force-pushed the e2e-local-npm branch 6 times, most recently from 89cb1d1 to e0f1c5c Compare May 31, 2022 18:32
@jbedard jbedard force-pushed the e2e-local-npm branch 4 times, most recently from 8742c1d to 60f0ec9 Compare June 1, 2022 19:59
@jbedard jbedard force-pushed the e2e-local-npm branch 3 times, most recently from 1424022 to 317f948 Compare June 2, 2022 21:05
@alan-agius4 alan-agius4 added the target: minor This PR is targeted for the next minor release label Jun 3, 2022
@jbedard jbedard requested a review from alan-agius4 June 3, 2022 23:45
@jbedard jbedard force-pushed the e2e-local-npm branch 3 times, most recently from db25a5b to d0548d6 Compare June 6, 2022 20:12
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Jun 7, 2022
@clydin clydin merged commit 9c26e28 into angular:main Jun 7, 2022
@clydin
Copy link
Member

clydin commented Jun 7, 2022

This appears to be causing failures for the full Windows CI job and the snapshots job:
https://app.circleci.com/pipelines/github/angular/angular-cli/23169/workflows/2aa69b28-122e-468f-9220-f7a36f6ae56c

@jbedard
Copy link
Contributor Author

jbedard commented Jun 7, 2022

Error: Running "cmd.exe /c git clean -dxf" returned error. 1...
warning: failed to remove .angular/cache/14.1.0-next.0/ng-packagr/content-v2/sha512/03/ce/bb4940cc77a94bd0f18e4752d30a7c2895fb086c8b48ae6258a7f2628e0c348bd1e42161f20f7efcbd9b6ff60dda64a1c9710bc3cb530e2831201a1e63d2: Filename too long
warning: failed to remove .angular/cache/14.1.0-next.0/ng-packagr/content-v2/sha512/18/eb/2ce695e9c32252f3fde4a56bcabc8bc319ed487f0afbc5322d41c5c8c9bee6079f160ca19345a84baa8a46e9339d1c1fae76e00465bebf52bf13434791db: Filename too long
warning: failed to remove .angular/cache/14.1.0-next.0/ng-packagr/content-v2/sha512/41/50/af2048670b4a59c036628768bde838d380c8e46d771b0f0893b8ba0af3d05b0de0ec1606973bb4bd8672cd6751d1db6e39d3671240d45c409b844fddae41: Filename too long
...

Any ideas here? Should that folder be gitignored and not be cleaned anyway?

@jbedard
Copy link
Contributor Author

jbedard commented Jun 7, 2022

I'll look into the snapshot ones more this morning.

What is run which wasn't run on the PR CI?

@clydin
Copy link
Member

clydin commented Jun 7, 2022

#23311 should fix the snapshot job. Windows will require more investigation though.

These are the jobs that are only run on main (Windows has a subset run on PRs):

  • e2e-cli-node-16
    <<: *only_release_branches
  • e2e-cli-ng-snapshots
    filters:
    branches:
    only:
    - renovate/angular
    - main
  • e2e-cli-win
    if (Test-Path env:CIRCLE_PULL_REQUEST) {
    node tests\legacy-cli\run_e2e.js "--glob={tests/basic/**,tests/i18n/extract-ivy*.ts,tests/build/profile.ts,tests/test/test-sourcemap.ts,tests/misc/check-postinstalls.ts}" --nb-shards=$env:CIRCLE_NODE_TOTAL --shard=$env:CIRCLE_NODE_INDEX
    } else {
    node tests\legacy-cli\run_e2e.js --nb-shards=$env:CIRCLE_NODE_TOTAL --shard=$env:CIRCLE_NODE_INDEX
    }

@alan-agius4
Copy link
Collaborator

alan-agius4 commented Jun 7, 2022

I briefly took a look from my mobile, what is strange is that the files not being removed are ng-packagr cache files, which strangely isn’t even run in the failings tests. Have these been left overs from previous tests?

I also noticed that some other tests are failing with task kill related errors

Error: Command failed: taskkill /pid 1892 /T /F
ERROR: The process "1892" not found.

@angular-automatic-lock-bot
Copy link

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 Jul 8, 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: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants