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

Make nimble component lint and test run concurrently #2065

Merged
merged 4 commits into from
May 2, 2024

Conversation

rajsite
Copy link
Member

@rajsite rajsite commented May 2, 2024

Pull Request

🤨 Rationale

Investigating locally I found that by far the nimble-components lint steps (both eslint and prettier) as well as the karma runs (chrome and firefox) are the slowest to run. Updated the concurrent run so that the component commands run in parallel from the start along with synchronous test and lint runs of every other package.

With this change the following commands all start at the same time:

npm run lint (runs lint sequentially one by one for each package in workspace order)
npm run test (runs test sequentially one by one for each package in workspace order)
npm run -w packages/nimble-components prettier (as part of npm run lint-concurrently)
npm run -w packages/nimble-components lint (as part of npm run lint-concurrently)
npm run -w packages/nimble-components test-firefox:verbose (as part of npm run test-concurrently)
npm run -w packages/nimble-components test-chrome:verbose (as part of npm run test-concurrently)

And can see in the following table that the last to finish are the components commands:

--> ┌───────────────────┬──────────┬───────────┬────────┬───────────────────────────┐
--> │ name              │ duration │ exit code │ killed │ command                   │
--> ├───────────────────┼──────────┼───────────┼────────┼───────────────────────────┤
--> │ lint-concurrently │ 317,863  │ 0         │ false  │ npm run lint-concurrently │
--> │ test-concurrently │ 311,984  │ 0         │ false  │ npm run test-concurrently │
--> │ test              │ 292,637  │ 0         │ false  │ npm run test              │
--> │ lint              │ 269,971  │ 0         │ false  │ npm run lint              │
--> └───────────────────┴──────────┴───────────┴────────┴───────────────────────────┘

[lint-concurrently] --> ┌──────────┬──────────┬───────────┬────────┬──────────────────┐
[lint-concurrently] --> │ name     │ duration │ exit code │ killed │ command          │
[lint-concurrently] --> ├──────────┼──────────┼───────────┼────────┼──────────────────┤
[lint-concurrently] --> │ prettier │ 316,767  │ 0         │ false  │ npm run prettier │
[lint-concurrently] --> │ eslint   │ 93,986   │ 0         │ false  │ npm run eslint   │
[lint-concurrently] --> └──────────┴──────────┴───────────┴────────┴──────────────────┘

[test-concurrently] --> ┌──────────────────────┬──────────┬───────────┬────────┬──────────────────────────────┐
[test-concurrently] --> │ name                 │ duration │ exit code │ killed │ command                      │
[test-concurrently] --> ├──────────────────────┼──────────┼───────────┼────────┼──────────────────────────────┤
[test-concurrently] --> │ test-firefox:verbose │ 310,925  │ 0         │ false  │ npm run test-firefox:verbose │
[test-concurrently] --> │ test-chrome:verbose  │ 303,807  │ 0         │ false  │ npm run test-chrome:verbose  │
[test-concurrently] --> └──────────────────────┴──────────┴───────────┴────────┴──────────────────────────────┘

In the end this ends up shaving off only about 2-3 minutes from the CI. I suspect we are probably at the point of diminishing returns for concurrent execution.

👩‍💻 Implementation

  • Renamed the lint and test commands for nimble components so they do not run in the normal lint and test execution for the workspace
  • Made the components lint and test commands run their substeps concurrently
  • Included the new concurrent component lint and test commands in the top level npm run concurrently-lint-test

🧪 Testing

Tested locally and CI.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@rajsite rajsite marked this pull request as ready for review May 2, 2024 01:29
@rajsite rajsite requested a review from jattasNI as a code owner May 2, 2024 01:29
@@ -0,0 +1,7 @@
{
Copy link
Member Author

Choose a reason for hiding this comment

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

@mollykreis would you mind buddying this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mollykreis I'm gonna merge this PR so we can get some runtime on it but will address concerns in a follow-up

"pack": "npm run pack --workspaces --if-present",
"change": "beachball change",
"check": "beachball check --changehint \"Run 'npm run change' to generate a change file\"",
"invoke-publish": "cross-env-shell beachball publish --yes --access public --message \\\"applying package updates [skip ci]\\\" -n $NPM_SECRET_TOKEN",
"validate": "npm run build && npm run lint && npm run test",
Copy link
Member Author

Choose a reason for hiding this comment

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

Intentionally removed, didn't think anyone really used it

@rajsite rajsite merged commit 561d153 into main May 2, 2024
13 checks passed
@rajsite rajsite deleted the concurrently-simple-pattern branch May 2, 2024 15:51
Comment on lines +19 to +20
"lint-concurrently": "npm run lint-concurrently -w packages/nimble-components",
"test-concurrently": "npm run test-concurrently -w packages/nimble-components",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do these two commands specific list the nimble-components package instead of using the --if-present flag?

Copy link
Member Author

@rajsite rajsite May 2, 2024

Choose a reason for hiding this comment

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

It's an intentional break from the pattern but I can see how it's being masked / not clear from the naming.

The sequential commands are fast and can afford to stack more on, the concurrent ones all need to start concurrently.

If we add a new concurrent command using --if-present it'll run the concurrent commands sequentially., i.e:

seqA -> seqB -> seqC -> end
concA -> concB -> concC -> end

Instead we want:

seqA -> seqB -> seqC -> end
concA -> end
concB -> end
concC -> end

I have a proposal that will make that more explicit (i.e. we specifically want components to run in parallel) in a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants