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

Jump to bottom

Set up Codecov for the project #671

Closed
4 of 5 tasks
nfischer opened this issue Feb 26, 2017 · 1 comment
Closed
4 of 5 tasks

Set up Codecov for the project #671

nfischer opened this issue Feb 26, 2017 · 1 comment
Assignees
@nfischer @freitagbr
Labels
blocked high priority test coverage

Comments

@nfischer
Copy link
Member

nfischer commented Feb 26, 2017 •
edited

Our test coverage is actually pretty decent, so now would be a good time to set up Codecov or coveralls for the project.

Some things we need to take a look at:

  • We should mark hard-to-test code blocks with /* istanbul ignore next */
  • echo tests appear to have horrible coverage because of how the tests are run. We'll need to resolve Echo tests unnecessarily run tests in own process #622 to make test coverage more accurate
  • Some lines are untested but should be tested. These cases are pretty trivial, I can take these on.
  • Some lines of code are actually dead code that are never reached. We might consider removing these at this point.
  • Actually add Codecov to the project 😄

Blocked on #622
nfischer added a commit that referenced this issue Feb 26, 2017
@nfischer
test: don't count hard-to-test lines for coverage
e4f57e7 
No change in logic.

Add `/* istanbul ignore next */` lines for hard-to-test lines so that
they don't count against us during code coverage.

I've also adjusted comments that I found confusing, and changed some
formatting.

Partial fix for #671
@nfischer nfischer mentioned this issue Feb 26, 2017
Merged
nfischer added a commit that referenced this issue Feb 26, 2017
@nfischer
test: add misc. tests to improve coverage
6e93e5c 
No change in production logic.

This adds missing tests to improve test coverage. This does not change
ShellJS behavior at all--all test cases are testing already-working
functionality.

One test case has been renamed for clarity.

For the "omit directory if missing recursive flag" case, we were
actually already testing that in another case, but we were testing
multiple things in that test case. It's better to test this one error
condition explicitly in its own case.

When adding real tests for `parseOptions()`, we need to explicitly clear
`common.state.error` because we're testing an internal function, not a
wrapped command.

Partial fix for #671
@nfischer nfischer mentioned this issue Feb 26, 2017
Merged
nfischer added a commit that referenced this issue Feb 26, 2017
@nfischer
refactor(parseOptions): better handle errors
a1080d7 
For the case where `parseOptions()` is passed a string that does not
start with a hyphen, we should reject this string instead of returning
the same value. This has no change on any other test cases, and should
not affect any commands since we are careful about what input we pass to
`parseOptions()`

This also adjust how we were handling error cases in the function. We
were previously using two different calls to `common.error()`, one for
if `errorOptions` is passed, and one without. This hurts readability,
because it reads like "if we have errorOptions, then this is an error,
and if we don't then it's also an error," instead of the more
appropriate, "we reached an error case and should use errorOptions if
it's available, otherwise we should signal an error without using it."

We do not need to use `errorOptions` for the added call to `error()`,
since this is an error which indicates misuse, and should not be
recoverable.

This adds a test case as well, for a line which was not previously
covered in our tests.

Partial fix for #671
This was referenced Feb 26, 2017
Merged
Closed
nfischer added a commit that referenced this issue Mar 4, 2017
@nfischer
test: don't count hard-to-test lines for coverage
e7fc8b7 
No change in logic.

Add `/* istanbul ignore next */` lines for hard-to-test lines so that
they don't count against us during code coverage.

I've also adjusted comments that I found confusing, and changed some
formatting.

Partial fix for #671
nfischer added a commit that referenced this issue Mar 4, 2017
@nfischer
test: don't count hard-to-test lines for coverage
4900568 
No change in logic.

Add `/* istanbul ignore next */` lines for hard-to-test lines so that
they don't count against us during code coverage.

I've also adjusted comments that I found confusing, and changed some
formatting.

Partial fix for #671
nfischer added a commit that referenced this issue Mar 4, 2017
@nfischer
chore: add codecov
da5ea21 
Add codecov to ShellJS

Partial fix for #671
@nfischer nfischer mentioned this issue Mar 4, 2017
Merged
freitagbr pushed a commit that referenced this issue Mar 5, 2017
@nfischer @freitagbr
chore: add codecov (#682)
eb5230a 
* chore: add codecov

Add codecov to ShellJS

Partial fix for #671

* Add codecov badge to README
freitagbr pushed a commit that referenced this issue Mar 5, 2017
@nfischer @freitagbr
test: don't count hard-to-test lines for coverage (#672)
346fca4 
No change in logic.

Add `/* istanbul ignore next */` lines for hard-to-test lines so that
they don't count against us during code coverage.

I've also adjusted comments that I found confusing, and changed some
formatting.

Partial fix for #671
nfischer added a commit that referenced this issue Mar 5, 2017
@nfischer
test: add misc. tests to improve coverage
2d02479 
No change in production logic.

This adds missing tests to improve test coverage. This does not change
ShellJS behavior at all--all test cases are testing already-working
functionality.

One test case has been renamed for clarity.

For the "omit directory if missing recursive flag" case, we were
actually already testing that in another case, but we were testing
multiple things in that test case. It's better to test this one error
condition explicitly in its own case.

When adding real tests for `parseOptions()`, we need to explicitly clear
`common.state.error` because we're testing an internal function, not a
wrapped command.

Partial fix for #671
nfischer added a commit that referenced this issue Mar 5, 2017
@nfischer
test: add misc. tests to improve coverage
34124cd 
No change in production logic.

This adds missing tests to improve test coverage. This does not change
ShellJS behavior at all--all test cases are testing already-working
functionality.

One test case has been renamed for clarity.

For the "omit directory if missing recursive flag" case, we were
actually already testing that in another case, but we were testing
multiple things in that test case. It's better to test this one error
condition explicitly in its own case.

When adding real tests for `parseOptions()`, we need to explicitly clear
`common.state.error` because we're testing an internal function, not a
wrapped command.

Partial fix for #671
nfischer added a commit that referenced this issue Mar 5, 2017
@nfischer
chore: add codecov script to appveyor CI
891d598 
Invoke codecov on appveyor builds too, so that we can get coverage for
Windows-specific lines of code.

Partial fix for #671
@nfischer nfischer mentioned this issue Mar 5, 2017
Merged
nfischer added a commit that referenced this issue Mar 5, 2017
@nfischer
chore: add codecov script to appveyor CI
30006dd 
Invoke codecov on appveyor builds too, so that we can get coverage for
Windows-specific lines of code.

Partial fix for #671
nfischer added a commit that referenced this issue Mar 5, 2017
@nfischer
chore: add codecov script to appveyor CI
18b4b82 
Invoke codecov on appveyor builds too, so that we can get coverage for
Windows-specific lines of code.

Partial fix for #671
nfischer added a commit that referenced this issue Mar 5, 2017
@nfischer
test: add misc. tests to improve coverage (#673)
35016ce 
No change in production logic.

This adds missing tests to improve test coverage. This does not change
ShellJS behavior at all--all test cases are testing already-working
functionality.

One test case has been renamed for clarity.

For the "omit directory if missing recursive flag" case, we were
actually already testing that in another case, but we were testing
multiple things in that test case. It's better to test this one error
condition explicitly in its own case.

When adding real tests for `parseOptions()`, we need to explicitly clear
`common.state.error` because we're testing an internal function, not a
wrapped command.

Partial fix for #671
freitagbr pushed a commit that referenced this issue Mar 5, 2017
@nfischer @freitagbr
chore: add codecov script to appveyor CI (#686)
fb09c6a 
Invoke codecov on appveyor builds too, so that we can get coverage for
Windows-specific lines of code.

Partial fix for #671
nfischer added a commit that referenced this issue Mar 8, 2017
@nfischer
refactor(parseOptions): better handle errors
208682c 
For the case where `parseOptions()` is passed a string that does not
start with a hyphen, we should reject this string instead of returning
the same value. This has no change on any other test cases, and should
not affect any commands since we are careful about what input we pass to
`parseOptions()`

This also adjust how we were handling error cases in the function. We
were previously using two different calls to `common.error()`, one for
if `errorOptions` is passed, and one without. This hurts readability,
because it reads like "if we have errorOptions, then this is an error,
and if we don't then it's also an error," instead of the more
appropriate, "we reached an error case and should use errorOptions if
it's available, otherwise we should signal an error without using it."

We do not need to use `errorOptions` for the added call to `error()`,
since this is an error which indicates misuse, and should not be
recoverable.

This adds a test case as well, for a line which was not previously
covered in our tests.

Partial fix for #671
freitagbr pushed a commit that referenced this issue Mar 9, 2017
@nfischer @freitagbr
refactor(parseOptions): better handle errors (#674)
54c1bef 
* refactor(parseOptions): better handle errors

For the case where `parseOptions()` is passed a string that does not
start with a hyphen, we should reject this string instead of returning
the same value. This has no change on any other test cases, and should
not affect any commands since we are careful about what input we pass to
`parseOptions()`

This also adjust how we were handling error cases in the function. We
were previously using two different calls to `common.error()`, one for
if `errorOptions` is passed, and one without. This hurts readability,
because it reads like "if we have errorOptions, then this is an error,
and if we don't then it's also an error," instead of the more
appropriate, "we reached an error case and should use errorOptions if
it's available, otherwise we should signal an error without using it."

We do not need to use `errorOptions` for the added call to `error()`,
since this is an error which indicates misuse, and should not be
recoverable.

This adds a test case as well, for a line which was not previously
covered in our tests.

Partial fix for #671

* Refactor parseOptions()

This validates input at the top of `parseOptions()` for type
correctness. This changes `typeof x === 'object'` to `x instanceOf
Object` to improve robustness. Now we can safely use `errorOptions`
knowing that if it has a truthy value, it will be an object, otherwise
we should use defaults instead.

The new tests ensure that we have 100% unit test coverage for this
function.

* Use exceptions, not common.error()s, for internal misuse

* Add common.isObject() helper function

The `isObject()` helper is used throughout common.js to reliably test if
variables are JavaScript objects.
@nfischer nfischer removed the blocked label Jun 17, 2017
@nfischer nfischer mentioned this issue Jun 17, 2017
Open
@nfischer
Copy link
Member Author

Before marking this as resolved, we should improve test coverage for the commands mentioned in these issues.

This was referenced Jun 17, 2017
Open
Open
Closed
Closed
nfischer added a commit that referenced this issue Jun 18, 2017
@nfischer
test(head): improve coverage
ad0b667 
This adds a test for `head()` on the right-hand side of a pipe. This also
removes the try-catch surrounding `fs.openSync()`, because it was unreachable
code. `fs.existsSync()` guarantees that the file exists, and `fs.openSync()`
only throws if the file does not exist, according to official documentation.

Fixes #671
@nfischer nfischer mentioned this issue Jun 18, 2017
Merged
@dependabot-preview dependabot-preview bot mentioned this issue Mar 19, 2018
Closed
@snyk-bot snyk-bot mentioned this issue Aug 13, 2019
Open
This was referenced Aug 29, 2019
Open
Open
@snyk-bot snyk-bot mentioned this issue Sep 19, 2019
Open
@snyk-bot snyk-bot mentioned this issue Nov 29, 2019
Open
@snyk-bot snyk-bot mentioned this issue Dec 18, 2019
Open
@snyk-bot snyk-bot mentioned this issue Mar 8, 2020
Open
This was referenced Mar 24, 2020
Merged
Merged
@snyk-bot snyk-bot mentioned this issue Apr 3, 2020
Open
This was referenced Apr 24, 2020
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nfischer nfischer

@freitagbr freitagbr

Labels
blocked high priority test coverage
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@nfischer @freitagbr