feat(linting): concurrent linting #6969

evenstensberg · 2023-08-20T14:37:59Z

Closes #6968

Describe your changes:

Adds concurrent linting. Note that some commands have been switched.

npm run lint is now lint:src
npm run lint:fix is now lint:fix so that eslint doesnt run twice on concurrent command lint:*
npm run lint:all is now npm run lint so that the run -p doesnt run recursively on itself.

Nice to know

Npm run all has better stdout with colors, which concurrently doesnt have
Concurrently does have exclude pattern, but it isn't better DX (Developer Experience)

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Command line build passes?
Has this been smoke tested?
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?
Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

deploysentinel · 2023-08-20T14:42:39Z

Current Playwright Test Results Summary

✅ 136 Passing - ⚠️ 2 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 08/30/2023 11:11:48pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: ceef077

Started: 08/30/2023 11:04:39pm UTC

⚠️ Flakes

📄 functional/plugins/plot/logPlot.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Log plot tests Log Plot ticks are functionally correct in regular and log mode and after refresh Retry 1 • Initial Attempt	3.13% (2) 2 / 64 runs failed over last 7 days	31.25% (20) 20 / 64 runs flaked over last 7 days

📄 functional/plugins/notebook/restrictedNotebook.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Restricted Notebook with a page locked and with an embed @addinit Disallows embeds to be deleted if page locked @addinit Retry 1 • Initial Attempt	0% (0) 0 / 60 runs failed over last 7 days	46.67% (28) 28 / 60 runs flaked over last 7 days

View Detailed Build Results

codecov · 2023-08-20T14:48:05Z

Codecov Report

Merging #6969 (4d4718e) into master (b87459d) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 4d4718e differs from pull request most recent head ceef077. Consider uploading reports for the commit ceef077 to get more accurate results

@@            Coverage Diff             @@
##           master    #6969      +/-   ##
==========================================
+ Coverage   48.23%   48.27%   +0.03%     
==========================================
  Files         643      643              
  Lines       25730    25721       -9     
  Branches     2512     2512              
==========================================
+ Hits        12412    12417       +5     
+ Misses      12840    12824      -16     
- Partials      478      480       +2

Flag	Coverage Δ		*Carryforward flag
e2e-full	`42.00% <ø> (+<0.01%)`	⬆️	Carriedforward from 5d287d8
e2e-stable	`57.14% <ø> (+0.13%)`	⬆️
unit	`38.68% <ø> (-0.03%)`	⬇️

*This pull request uses carry forward flags. Click here to find out more.

see 10 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b87459d...ceef077. Read the comment docs.

unlikelyzero

@evenstensberg this is great! Since we're overriding one of the protected terms 'lint', We will need to check github actions and circleci for cli flags

run-p lint:* --quiet

Fails. Will need to be npm run lint -- --quiet

evenstensberg · 2023-08-22T20:07:30Z

I can make --quiet work on everything but the cspell linter.

Opened a issue here: streetsidesoftware/cspell#4742

For now customized the two scripts to run a ci version of the linting.

evenstensberg · 2023-08-22T20:08:19Z

package.json

    "lint:spelling": "cspell \"**/*.{js,md,vue}\" --show-context --gitignore",
-    "lint:fix": "eslint example src e2e --ext .js,.vue openmct.js --fix",
+    "lint": "run-p lint:*",
+    "ci": "run-p \"lint:src -- --quiet\" \"lint:spelling -- --fail-fast --no-progress\"",


Make sure the name is correct. Can change to ci:lint for instance @unlikelyzero

this is getting pretty complex. The reason we added --quiet was so that the official github reporter output wouldn't explode on in-line output on PRs. Maybe that's no longer necessary.

Let's try:

remove the --quiet flag with this the previous implementation on github workflows

Update a linter with a true positive failure

run pr:platform

and verify that PRs aren't poluted with 12 versions of the same failed linter rule inline on the code

evenstensberg · 2023-08-22T20:42:22Z

@unlikelyzero without quiet flag. This is what we don't want, right?

evenstensberg · 2023-08-22T21:00:48Z

Could do this: eslint/eslint#13898 (comment)

unlikelyzero · 2023-08-23T05:17:39Z

@evenstensberg ok i found the original card: #4909.

Here's an example of what it looks like when the github in-line reporter goes nuts. The following example is the pr:e2e workflow https://github.com/nasa/openmct/pull/5012/files

Effectively we turned on quiet mode to prevent the github inline reporter from creating 10+ testfailures in the /pull/#/files view. instead of worrying about quiet output, maybe we can just figure out how to prevent the github annotation reporter from spamming the files view for failed linting steps.

.github/workflows/pr-platform.yml

evenstensberg · 2023-08-23T20:19:27Z

@unlikelyzero I'll investigate if we can suppress through github workflow (probably due next week)

evenstensberg · 2023-08-24T19:42:30Z

@unlikelyzero I've spoken with the maintainers of cspell and they are discussing adding quietas an alias for silent. This means that we can keep the -quietflag as you have previously

evenstensberg · 2023-08-27T20:37:22Z

PR is merged (streetsidesoftware/cspell#4748) and new release on Monday 28th of August. Will update the dependency when new version is rolled out.

evenstensberg · 2023-08-28T16:48:16Z

@unlikelyzero It seems like passing a boolean to the --quiet flag doesnt get recognized, nor is passing quiet as a param in the cli. so we'll need to add another script to run in CI.

Is that ok? The PR is ready to be reviewed.

unlikelyzero · 2023-08-28T17:53:06Z

@evenstensberg I am on PTO today. Can you see if it's possible to silence GitHub annotation reporter for the results and we can remove the quiet flags altogether?

evenstensberg · 2023-08-28T18:42:59Z

@unlikelyzero For reference, I managed to make it work. The script lint looks a bit hacky, but it allows to optionally pass a flag/option to each of the scripts and in this case --quiet.

Enjoy your day, and lmk if you need something 👍🏾

unlikelyzero · 2023-08-29T21:39:19Z

package.json

    "lint:spelling": "cspell \"**/*.{js,md,vue}\" --show-context --gitignore",
-    "lint:fix": "eslint example src e2e --ext .js,.vue openmct.js --fix",
+    "lint": "run-p \"lint:* -- {1}\" --",
+    "fix:lint": "eslint example src e2e --ext .js,.vue openmct.js --fix",


@evenstensberg after some internal discussion, we think fix:lint should be reverted to lint:fix

We will then need to explicitly list the scripts in lint as

"lint":"run-p lint:js lint:vue lint:spelling"

Lastly, it appears that npm-run-all is no longer maintained and we will want to switch to npm-run-all2

package.json

unlikelyzero

We completed the final review. A few changes, but otherwise this is great and opens the door for more linting options

evenstensberg · 2023-08-30T16:21:17Z

@unlikelyzero review change done. Personally I think that the runp lint:* looks cleaner

evenstensberg · 2023-08-30T16:51:06Z

@unlikelyzero also sent a mail to arc-dl. Would be nice if you had a look and replied.

ozyx · 2023-08-30T16:58:55Z

@unlikelyzero also sent a mail to arc-dl. Would be nice if you had a look and replied.

Hi @evenstensberg ,

We've received your message and will be responding today, please stand by. Appreciate your patience!

unlikelyzero · 2023-08-30T22:32:25Z

package.json

@@ -54,6 +54,7 @@
    "moment": "2.29.4",
    "moment-duration-format": "2.3.2",
    "moment-timezone": "0.5.41",
+    "npm-run-all2": "^6.0.6",


pin this to latest

unlikelyzero

This looks great! Just one change (pin npm-run-all2 to the latest) and then we merge this in

feat(linting): concurrent linting

e42fc43

evenstensberg added 2 commits August 20, 2023 18:02

fix(deps): drop concurrently

beb4193

chore(deps): lock dep

0afb5ba

ozyx added the source:community Community contribution or request label Aug 20, 2023

Merge branch 'master' into feat/concurrent-linting

b35abb7

unlikelyzero added pr:e2e Automatically triggers e2e tests to run pr:platform Runs tests against all supported platforms pr:e2e:couchdb npm run test:e2e:couchdb labels Aug 22, 2023

unlikelyzero added this to the Target:3.1.0 milestone Aug 22, 2023

unlikelyzero self-requested a review August 22, 2023 16:13

unlikelyzero requested changes Aug 22, 2023

View reviewed changes

evenstensberg commented Aug 22, 2023

View reviewed changes

evenstensberg force-pushed the feat/concurrent-linting branch from db1e92c to b35abb7 Compare August 22, 2023 20:36

fix: remove quiet flag

67bd098

unlikelyzero reviewed Aug 23, 2023

View reviewed changes

.github/workflows/pr-platform.yml Outdated Show resolved Hide resolved

evenstensberg added 2 commits August 28, 2023 18:46

fix(ci): quiet flag

1d65980

fix(ci): quiet flag

5e478ee

evenstensberg requested a review from unlikelyzero August 28, 2023 17:46

fix: make quiet possible

8f97b71

evenstensberg added 2 commits August 28, 2023 20:41

chore: reset file

2c0812c

chore: reset file

c0cb005

unlikelyzero reviewed Aug 29, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

unlikelyzero requested changes Aug 29, 2023

View reviewed changes

fix: review

5d287d8

evenstensberg requested a review from unlikelyzero August 30, 2023 16:20

fix: old commit

4d4718e

unlikelyzero reviewed Aug 30, 2023

View reviewed changes

unlikelyzero approved these changes Aug 30, 2023

View reviewed changes

evenstensberg and others added 2 commits August 31, 2023 01:01

chore: pin

a0026e5

Merge branch 'master' into feat/concurrent-linting

ceef077

unlikelyzero enabled auto-merge (squash) August 30, 2023 23:02

unlikelyzero added pr:e2e:couchdb npm run test:e2e:couchdb and removed pr:e2e Automatically triggers e2e tests to run pr:platform Runs tests against all supported platforms pr:e2e:couchdb npm run test:e2e:couchdb labels Aug 30, 2023

unlikelyzero merged commit e09a7ae into nasa:master Aug 30, 2023
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(linting): concurrent linting #6969

feat(linting): concurrent linting #6969

evenstensberg commented Aug 20, 2023

deploysentinel bot commented Aug 20, 2023 •

edited

Test Case Results

Test Case Results

codecov bot commented Aug 20, 2023 •

edited

unlikelyzero left a comment •

edited

evenstensberg commented Aug 22, 2023

evenstensberg Aug 22, 2023

unlikelyzero Aug 22, 2023

evenstensberg commented Aug 22, 2023

evenstensberg commented Aug 22, 2023

unlikelyzero commented Aug 23, 2023

evenstensberg commented Aug 23, 2023

evenstensberg commented Aug 24, 2023

evenstensberg commented Aug 27, 2023

evenstensberg commented Aug 28, 2023 •

edited

unlikelyzero commented Aug 28, 2023

evenstensberg commented Aug 28, 2023

unlikelyzero Aug 29, 2023

unlikelyzero left a comment

evenstensberg commented Aug 30, 2023

evenstensberg commented Aug 30, 2023

ozyx commented Aug 30, 2023

unlikelyzero Aug 30, 2023

unlikelyzero left a comment

feat(linting): concurrent linting #6969

feat(linting): concurrent linting #6969

Conversation

evenstensberg commented Aug 20, 2023

Describe your changes:

Nice to know

All Submissions:

Author Checklist

Reviewer Checklist

deploysentinel bot commented Aug 20, 2023 • edited

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

Test Case Results

codecov bot commented Aug 20, 2023 • edited

Codecov Report

unlikelyzero left a comment • edited

Choose a reason for hiding this comment

evenstensberg commented Aug 22, 2023

evenstensberg Aug 22, 2023

Choose a reason for hiding this comment

unlikelyzero Aug 22, 2023

Choose a reason for hiding this comment

evenstensberg commented Aug 22, 2023

evenstensberg commented Aug 22, 2023

unlikelyzero commented Aug 23, 2023

evenstensberg commented Aug 23, 2023

evenstensberg commented Aug 24, 2023

evenstensberg commented Aug 27, 2023

evenstensberg commented Aug 28, 2023 • edited

unlikelyzero commented Aug 28, 2023

evenstensberg commented Aug 28, 2023

unlikelyzero Aug 29, 2023

Choose a reason for hiding this comment

unlikelyzero left a comment

Choose a reason for hiding this comment

evenstensberg commented Aug 30, 2023

evenstensberg commented Aug 30, 2023

ozyx commented Aug 30, 2023

unlikelyzero Aug 30, 2023

Choose a reason for hiding this comment

unlikelyzero left a comment

Choose a reason for hiding this comment

deploysentinel bot commented Aug 20, 2023 •

edited

codecov bot commented Aug 20, 2023 •

edited

unlikelyzero left a comment •

edited

evenstensberg commented Aug 28, 2023 •

edited