-
Couldn't load subscription status.
- Fork 119
ncu-ci: command that shows results of jenkins #161
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
Conversation
|
Refs: #158 Some other commands to implement
|
Codecov Report
@@ Coverage Diff @@
## master #161 +/- ##
==========================================
- Coverage 88.52% 79.77% -8.75%
==========================================
Files 19 20 +1
Lines 723 1078 +355
==========================================
+ Hits 640 860 +220
- Misses 83 218 +135
Continue to review full report at Codecov.
|
1d14de5 to
18fcbe7
Compare
|
On Windows Refs: nodejs/node#19041 |
9d8b2e1 to
e963f1f
Compare
|
Added tests to cache and the jenkins utilities...should be able to land after documentations are added |
7996277 to
4451d21
Compare
- Put the PR URL parsing logic in links.js and reuse it - Differentiate PR and commit jobs in preparation for #161 - Updates a few CI link patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work!
|
@joyeecheung I can help with documentation if you want. |
bfc7e0f to
72782dd
Compare
|
Docs added, new demo: https://asciinema.org/a/184727 This should be ready to land cc @nodejs/automation |
|
This has been approved by @mmarchini but I still prefer to have some reviews @nodejs/automation before merging. It would be nice to be able to land this before tomorrow's collaboration summit so I can make a release and people can start trying this out - currently I believe the pattern matching of the CI results are not smart enough and we will bump into edge cases from time to time. The more people trying this the better coverage we can have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left couple of comments, overall LGTM, Nice work!
I still quite did not get what the purpose of the cache module, just by reading trough it?
| class Cache { | ||
| constructor(dir) { | ||
| this.dir = dir || path.join(__dirname, '..', '.ncu', 'cache'); | ||
| this.originals = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd would have prefer this to be a Map instead of plain object, for readability.
test/unit/cache.test.js
Outdated
| } | ||
| }); | ||
|
|
||
| it('should cache sync restuls', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, results.
test/unit/cache.test.js
Outdated
| assert.strictEqual(cached, expected); | ||
| }); | ||
|
|
||
| it('should cache async restuls', async() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto.
test/unit/ci.test.js
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('Jeknins', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo Jenkins.
| PRBuild, BenchmarkRun, CommitBuild, jobCache, parseJobFromURL, constants | ||
| PRBuild, BenchmarkRun, CommitBuild, parseJobFromURL, | ||
| constants, JobParser | ||
| // , jobCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was left by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, i was looking trough each commit.
| // This is used for testing | ||
| // Default cache dir is ${ncu-source-dir}/.ncu/cache | ||
| jobCache.enable(); | ||
| // jobCache.enable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need get rid this block now that this is not required for tests?
|
@priyankp10 Thanks for the review! The cache is there for development: e.g. when you are working on the pattern-matching rules you would not want to wait for the tool to grab all the data from Jenkins each time you run it. |
- Put the PR URL parsing logic in links.js and reuse it - Differentiate PR and commit jobs in preparation for nodejs/node-core-utils#161 - Updates a few CI link patterns
- Put the PR URL parsing logic in links.js and reuse it - Differentiate PR and commit jobs in preparation for nodejs/node-core-utils#161 - Updates a few CI link patterns
- Put the PR URL parsing logic in links.js and reuse it - Differentiate PR and commit jobs in preparation for nodejs/node-core-utils#161 - Updates a few CI link patterns
- Put the PR URL parsing logic in links.js and reuse it - Differentiate PR and commit jobs in preparation for nodejs/node-core-utils#161 - Updates a few CI link patterns
- Put the PR URL parsing logic in links.js and reuse it - Differentiate PR and commit jobs in preparation for nodejs/node-core-utils#161 - Updates a few CI link patterns

Example: