-
Notifications
You must be signed in to change notification settings - Fork 114
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
ncu-ci: command that shows results of jenkins #161
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?
lib/cache.js
Outdated
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
@@ -55,3 +63,66 @@ describe('JobParser', () => { | |||
assert.deepStrictEqual([...expected.entries()], [...results.entries()]); | |||
}); | |||
}); | |||
|
|||
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
.
bin/ncu-ci
Outdated
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.
bin/ncu-ci
Outdated
const Request = require('../lib/request'); | ||
const CLI = require('../lib/cli'); | ||
const yargs = require('yargs'); | ||
|
||
// 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: