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

chore: split CI builds to new files #464

Merged
merged 1 commit into from
Aug 6, 2020

Conversation

codebytere
Copy link
Member

Follow-up to #455:

I also did a bit of a refactor here, since there were a million classes in the result parser file and it was getting wildly unwieldy. I tried to do the minimum necessary to make this work but I plan to follow it up with the remainder of the the class split-out work.

This is the remainder of that work - the file was inscrutably long and so i felt this would be more maintainable in this form. Specs are passing, but please take a good look if you can to make sure i didn't whiff on anything!

cc @nodejs/node-core-utils

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #464 into master will increase coverage by 5.69%.
The diff coverage is 65.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   76.88%   82.58%   +5.69%     
==========================================
  Files          28       34       +6     
  Lines        1752     1665      -87     
==========================================
+ Hits         1347     1375      +28     
+ Misses        405      290     -115     
Impacted Files Coverage Δ
lib/ci/ci_utils.js 36.84% <14.28%> (-63.16%) ⬇️
lib/ci/build-types/fanned_build.js 18.42% <18.42%> (ø)
lib/ci/build-types/linter_build.js 26.66% <26.66%> (ø)
lib/links.js 88.73% <50.00%> (-1.13%) ⬇️
lib/ci/build-types/commit_build.js 79.31% <79.31%> (ø)
lib/ci/build-types/normal_build.js 80.95% <80.95%> (ø)
lib/ci/build-types/benchmark_run.js 87.80% <87.80%> (ø)
lib/ci/build-types/test_run.js 88.88% <88.88%> (ø)
lib/ci/build-types/pr_build.js 92.68% <92.68%> (ø)
lib/pr_checker.js 95.49% <100.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

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

@codebytere codebytere requested a review from a team August 4, 2020 18:26
@priyank-p
Copy link
Contributor

Not saying this needs to be done this way, but I think doing this refactor one or couple of class per commit would have made reviewing this way easier. Less changes to look at per commit means it is easier to spot a mistake :)

On the contrary, if you were missing an import of a class then eslint would have complained but doing it that way is still better.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Rubberstamp LGTM :)

@priyank-p priyank-p merged commit fd566e2 into nodejs:master Aug 6, 2020
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.

3 participants