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

extract checking AMD bundle as own test command #3214

Merged
merged 1 commit into from
Mar 21, 2018
Merged

extract checking AMD bundle as own test command #3214

merged 1 commit into from
Mar 21, 2018

Conversation

outsideris
Copy link
Member

Description of the Change

As #3208 , I moved checking AMD bundle into package-script.js.
I'm not sure the name which istest.browser.amd is properly.

Alternate Designs

I considered between nps lint.amd and npm test.browser.amd.
I thought it is a kind of testing rather than linting.

Why should this be in core?

See #3208

Benefits

Developers can run it locally.

Possible Drawbacks

N/A

Applicable issues

Fix #3208

@jsf-clabot
Copy link

jsf-clabot commented Jan 19, 2018 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling 0c367ec on outsideris:issue-3208 into 2e7e4c0 on mochajs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 90.018% when pulling 2fc1f64 on outsideris:issue-3208 into 2e7e4c0 on mochajs:master.

@coveralls
Copy link

coveralls commented Jan 19, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 90.086% when pulling 51a9116 on outsideris:issue-3208 into 19b764d on mochajs:master.

boneskull
boneskull suggested changes Jan 22, 2018
View reviewed changes
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

thanks! I'm not sure if I was entirely clear in the issue, but what I'm looking for here is an actual .spec.js test file. as written, the test will run in to trouble on non-POSIX OSes.

package-scripts.js Outdated
@@ -185,6 +185,10 @@ module.exports = {
esm: {
script: 'MOCHA_TEST=esm nps test.browser.unit',
description: 'Test mocha ESM support'
},
amd: {
script: 'if grep -q "define.amd" mocha.js; then exit 1; fi',
Copy link
Member

Choose a reason for hiding this comment

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

this will likely fail in windows. so maybe better to write an actual test (put intest/ somewhere) which:

  1. triggers npm start build to ensure mocha.js is present
  2. reads mocha.js into a string via fs.readFile()
  3. asserts "define.amd" is not present in the string

what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I got what you mean. It makes sense and looks like more maintainable.
I will try to fix it.

@boneskull boneskull added qa semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Jan 22, 2018
@outsideris
Copy link
Member Author

I rewrote the test case to depend on node.js rather than shell script.

@outsideris
Copy link
Member Author

@boneskull Could you check this?

boneskull
boneskull suggested changes Mar 20, 2018
View reviewed changes
Copy link
Member

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

thanks, I have a couple comments

package-scripts.js Outdated
@@ -185,6 +185,10 @@ module.exports = {
esm: {
script: 'MOCHA_TEST=esm nps test.browser.unit',
description: 'Test mocha ESM support'
},
bundle: {
Copy link
Member

Choose a reason for hiding this comment

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

this isn't really a browser test because we're not invoking Karma. maybe should just move it to be its own thing, e.g. test.bundle. also, there's a test() function to help.

test/browser-specific/bundle.spec.js Outdated

it('should build a non-broken bundle for AMD', function (done) {
var bundle = path.join(process.cwd(), 'mocha.js');
fs.readFile(bundle, function (err, data) {
Copy link
Member

Choose a reason for hiding this comment

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

specify utf8 encoding to get a string

test/browser-specific/bundle.spec.js Outdated
fs.readFile(bundle, function (err, data) {
if (err) { return done(err); }

var content = data.toString();
Copy link
Member

Choose a reason for hiding this comment

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

it might be more straightforward to use expect(content).not.to.match(/define.amd/) or something like this

@outsideris
Copy link
Member Author

I moved the test under test.bundle.amd and fixed it as feedbacks.

@boneskull boneskull added this to the next milestone Mar 21, 2018
@boneskull boneskull merged commit f4275b6 into mochajs:master Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boneskull boneskull boneskull approved these changes

Assignees
No one assigned
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes"
Projects
None yet
Milestone
v5.0.5
Development

Successfully merging this pull request may close these issues.

travis "before script" should only be run once
4 participants
@outsideris @jsf-clabot @coveralls @boneskull