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.

Sign up for GitHub

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

feat: --100 #332

Merged
merged 11 commits into from
Oct 6, 2021
Merged

feat: --100 #332

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
66cd626
test: --100
gr2m Oct 1, 2021
3aecef0
WIP attempt to use YARGS middleware to set branches/functions/lines/s…
gr2m Oct 1, 2021
b96dcda
comment out `yargs.middleware()` and add TODO comment
gr2m Oct 4, 2021
279276e
add `--100` workaround to `lib/commands/report.js` until the upgrade …
gr2m Oct 4, 2021
1dc1063
update snapshot for `c8 check-coverage --100` test
gr2m Oct 4, 2021
79bc20c
remove `console.log()`s used for debugging
gr2m Oct 4, 2021
244af62
remove extra space from test label
gr2m Oct 4, 2021
90505ec
check-coverage command with --100
gr2m Oct 4, 2021
c4286d1
set coverage for statements to 100%, too
gr2m Oct 4, 2021
cb7c7ed
docs(README): `--100`
gr2m Oct 4, 2021
08dff42
build: update Node 10 snapshots
bcoe Oct 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/parse-args.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ function buildYargs (withCommands = false) {
description: 'check thresholds per file',
type: 'boolean'
})
.option('100', {
default: false,
group: 'Coverage thresholds',
description: 'shortcut for --check-coverage --lines 100 --functions 100 --branches 100 --statements 100',
type: 'boolean'
})
.option('temp-directory', {
describe: 'directory V8 coverage data is written to and read from',
default: process.env.NODE_V8_COVERAGE
Expand Down Expand Up @@ -145,6 +151,18 @@ function buildYargs (withCommands = false) {
})
.epilog('visit https://git.io/vHysA for list of available reporters')

yargs.middleware((argv) => {
if (!argv[100]) return argv
gr2m marked this conversation as resolved.
Show resolved Hide resolved

return {
...argv,
branches: 100,
functions: 100,
lines: 100,
statements: 100,
}
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this code doesn't seem to be executed. I haven't worked with yargs middleware before but it seems like a good approach here? Do you know why the code is not executed?

Copy link
Owner

Choose a reason for hiding this comment

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

This seems like a reasonable approach to me, and seems like it should work, perhaps:


yargs.middleware((argv) => {
    if (!argv['100']) return argv

    return {
      ...argv,
      branches: 100,
      functions: 100,
      lines: 100,
      statements: 100,
    }
}

I wouldn't expect the argument key itself to be coerced into an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my problem here is that the middleware is not applied at all. I think I'm missing something obvious. Any idea what it could be?

Copy link
Owner

Choose a reason for hiding this comment

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

I tested locally, it seems like this is a bug that I fixed in yargs@17, at least it works as expected with yargs@17 ...

Unfortunately, I think we might want to hold off on dropping Node 10 until I can drop Node 10 on the libraries I manage at Google.

We can implement a less elegant version here:

https://github.com/bcoe/c8/blob/main/lib/commands/report.js#L29

Which I believe should work in the appropriate contexts (we'll want to confirm it works for the check-coverage command).

Let's add a // TODO: comment to switch to the middleware approach as soon as we can drop Node 10?


const checkCoverage = require('./commands/check-coverage')
const report = require('./commands/report')
if (withCommands) {
Expand Down
17 changes: 17 additions & 0 deletions test/integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,23 @@ describe('c8', () => {
status.should.equal(1)
output.toString('utf8').should.matchSnapshot()
})

it('--100 ', () => {
const { output, status } = spawnSync(nodePath, [
c8Path,
'--exclude="test/*.js"',
'--temp-directory=tmp/check-coverage',
'--100',
nodePath,
require.resolve('./fixtures/normal')
])
console.log(`output`);
console.log(output.toString("utf8"));
gr2m marked this conversation as resolved.
Show resolved Hide resolved

status.should.equal(1)
output.toString('utf8').should.matchSnapshot()

})
})

describe('report', () => {
Expand Down
19 changes: 19 additions & 0 deletions test/integration.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,25 @@ All files | 100 | 100 | 100 | 100 |
,"
`;

exports[`c8 check-coverage --100 1`] = `
",hey
i am a line of code
what
hey
what
hey
what
hey
-----------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-----------|---------|----------|---------|---------|-------------------
All files | 83.33 | 85.71 | 60 | 83.33 |
async.js | 100 | 100 | 100 | 100 |
normal.js | 75 | 66.66 | 33.33 | 75 | 14-16,18-20
-----------|---------|----------|---------|---------|-------------------
,"
`;

exports[`c8 check-coverage allows --check-coverage when executing script 1`] = `
",hey
i am a line of code
Expand Down