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

cleanup(angular): consolidate and improve e2e-angular-core tests #15726

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

leosvelperez
Copy link
Member

@leosvelperez leosvelperez commented Mar 17, 2023

What it does

  • Remove some duplicated work:
    • Multiple tests covering the same task runs with no added value
    • Generation of new projects when the existing ones could be reused
  • Remove an invalid test suite that's testing something we don't target to support ng-cli.test.ts
  • Improve ports management/cleanup
  • Add caching to creating Angular CLI workspaces

With the changes in the PR, the time is reduced, but it's still insufficient to meet the target. The last run did end up below the 15-minute mark (14m 27s), but there's currently a test case that's temporarily disabled which will bring the time up once enabled. In a separate branch/PR I'll explore splitting the test project.

The latest run took 12m 56s, so we seem to meet the target now. We'll keep an eye on it.

@leosvelperez leosvelperez self-assigned this Mar 17, 2023
@vercel
Copy link

vercel bot commented Mar 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
nx-dev ⬜️ Ignored (Inspect) Mar 28, 2023 at 10:05AM (UTC)

@leosvelperez leosvelperez force-pushed the angular/speed-up-e2e-tests branch 3 times, most recently from 090385a to 85aee88 Compare March 20, 2023 13:26
@leosvelperez leosvelperez changed the title cleanup(angular): speed up e2e-angular-core tests cleanup(angular): consolidate e2e-angular-core tests Mar 20, 2023
@leosvelperez leosvelperez changed the title cleanup(angular): consolidate e2e-angular-core tests cleanup(angular): consolidate and improve e2e-angular-core tests Mar 20, 2023
@leosvelperez leosvelperez marked this pull request as ready for review March 20, 2023 14:51
@meeroslav
Copy link
Contributor

@leosvelperez, when you look at the tests and their timings:

Total time: 1057306ms = ~17min

Test details
angular-linting.test.ts (70.254 s)
config.test.ts (71.819 s)
module-federation.test.ts (297.171 s)
ng-add.test.ts (345.513 s)
ng-cli.test.ts (33.616 s)
projects.test.ts (235.823 s)

You can spot several bottlenecks:

  • Linting does not need to be a separate suite, and can reuse projects/apps/libs from e.g. projects.test.ts (at 'should generate an app, a lib, link them, build, serve and test both correctly'). This would likely save a minute of work
  • Same goes for Angular Config that just runs on a default application (same like linter)
  • On ng-add we keep running NgNew for each test which is a very expensive operation. We could apply the same caching logic as we have for newProject where we would generate it only once in the temp folder and then copy it over for each subsequent test.
  • Module federation could be a separate e2e project

@leosvelperez
Copy link
Member Author

@meeroslav thanks for the tips! Those are great and I will work on applying them.

As mentioned in the PR description, I'll split the tests on a separate PR. I also want to check the angular-extensions project and split them more evenly, so I'll keep that separate.

@leosvelperez leosvelperez marked this pull request as draft March 21, 2023 09:06
@meeroslav
Copy link
Contributor

Please note that each test project splitting leads to a multiplication of git checkout, yarn install, verdaccio publish...

This should truly be only the last resort and only makes sense for test projects that take longer than 10 minutes since the preparation part often takes up to 3-4 minutes.

@leosvelperez
Copy link
Member Author

Please note that each test project splitting leads to a multiplication of git checkout, yarn install, verdaccio publish...

This should truly be only the last resort and only makes sense for test projects that take longer than 10 minutes since the preparation part often takes up to 3-4 minutes.

That is why I'm keeping it as a separate change and don't want to just extract the MF tests into a project. If it comes to splitting the projects, it would be better to split the current two projects into three to have a more even split.

@leosvelperez leosvelperez force-pushed the angular/speed-up-e2e-tests branch from 85aee88 to 5ac4689 Compare March 22, 2023 16:01
@leosvelperez leosvelperez marked this pull request as ready for review March 22, 2023 17:31
@leosvelperez
Copy link
Member Author

leosvelperez commented Mar 22, 2023

@meeroslav I addressed the changes you requested except:

  • Splitting Module Federation tests into a separate test project, as mentioned previously, splitting the current Angular e2e tests is something I might explore in a different PR, though the last run for this PR was 12m 56s, so we are on target, at least for now
  • Merging the Angular Config test suite with another. We're keeping this one separate. Its setup is not that of a regular workspace because we use angular.json with v1 format. It's not impossible to merge it with another suite, but I rather not do it. It'd be messy with the setup and cleanup between tests and affect the readability of the test suite.

Additionally, I removed an additional test suite I realized was testing something we don't really target to support, so we shouldn't be testing it.

@leosvelperez leosvelperez requested a review from meeroslav March 22, 2023 17:42
@leosvelperez leosvelperez force-pushed the angular/speed-up-e2e-tests branch from 5ac4689 to b7fa75d Compare March 23, 2023 17:43
Copy link
Contributor

@meeroslav meeroslav left a comment

Choose a reason for hiding this comment

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

Great work! Thank you!

@leosvelperez leosvelperez force-pushed the angular/speed-up-e2e-tests branch from b7fa75d to fed7b0c Compare March 28, 2023 10:04
@FrozenPandaz FrozenPandaz merged commit 9dbc90d into nrwl:master Mar 28, 2023
@leosvelperez leosvelperez deleted the angular/speed-up-e2e-tests branch March 28, 2023 16:55
@github-actions
Copy link

github-actions bot commented Apr 3, 2023

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants