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

test: add module require tests for certain package.json errors #23285

Closed
wants to merge 2 commits into from

Conversation

TomCoded
Copy link
Contributor

@TomCoded TomCoded commented Oct 5, 2018

test for unusual error cases: verify that module require
falls back to index if package.json names a missing file and
throws an error if package.json is unparseable.

Checklist
  • make -j4 test (UNIX) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

test for unusual error cases: verify that module require()
falls back to index if package.json names a missing file and
throws an error if package.json is unparseable.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 5, 2018
} catch (e) {
unparseableErrorThrown = true;
assert.strictEqual(/^Error parsing .*/.test(e.message), true);
}
Copy link
Member

Choose a reason for hiding this comment

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

Lines 109 through 115 and line 330 should probably be removed and replaced with a call to assert.throws():

assert.throws(
  () => { require('../fixtures/packages/unparseable'); },
  /^Error parsing /
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@Trott
Copy link
Member

Trott commented Oct 5, 2018

Not a big deal but maybe for next time something like this arises: Might be a good idea to split these two additions into separate commits. If one test case ends up being reverted or something, it won't take the other test one with it.

@Trott
Copy link
Member

Trott commented Oct 5, 2018

What motivated adding these tests? Were you reviewing coverage statistics and trying to improve coverage? If not, how did you determine that there wasn't an existing test case for these situations?

@TomCoded
Copy link
Contributor Author

TomCoded commented Oct 6, 2018

Some unusual require() behavior was noticed in a while back in #22464 and we updated the docs. I took a look at the code and the test coverage and did not see tests for some of the edge cases.

@Trott
Copy link
Member

Trott commented Oct 6, 2018

@nodejs/testing

// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
// USE OR OTHER DEALINGS IN THE SOFTWARE.

exports.ok = 'ok';
Copy link
Member

Choose a reason for hiding this comment

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

Oh, can you remove all the copyright boilerplate for this file? That copyright boilerplate is only necessary for files that already have it or new files that are substantially derived from existing files with the copyright. Since this is just one line, I think we're in the clear.

Copy link
Member

Choose a reason for hiding this comment

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

(Uh, unless you did in fact copy this from an existing file, in which case, leave the copyright. :-D )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) I left it since it is identical to several other files (so that the tests are consistent in design) and at least arguably is a copy of them.

Copy link
Member

Choose a reason for hiding this comment

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

:) I left it since it is identical to several other files (so that the tests are consistent in design) and at least arguably is a copy of them.

Sounds good to me. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

New files should not contain the notice.

Copy link
Member

@Trott Trott Oct 6, 2018

Choose a reason for hiding this comment

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

New files that, like this one, are exact copies of existing files should preserve the notice.

This file is an exact copy of test/fixtures/packages/main/package-main-module.js and test/fixtures/packages/main-index/package-main-module/index.js.

@Trott
Copy link
Member

Trott commented Oct 6, 2018

@Trott
Copy link
Member

Trott commented Oct 8, 2018

Landed in 13e6e01. Thanks!

@Trott Trott closed this Oct 8, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Oct 8, 2018
test for unusual error cases: verify that module require()
falls back to index if package.json names a missing file and
throws an error if package.json is unparseable.

PR-URL: nodejs#23285
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
test for unusual error cases: verify that module require()
falls back to index if package.json names a missing file and
throws an error if package.json is unparseable.

PR-URL: #23285
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
test for unusual error cases: verify that module require()
falls back to index if package.json names a missing file and
throws an error if package.json is unparseable.

PR-URL: #23285
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants