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

chore(build): use new config format for mocha v6.x #2309

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Jan 30, 2019

Supersedes #2281.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@raymondfeng
Copy link
Contributor Author

Need to investigate why (node:1888) ExperimentalWarning: The http2 module is an experimental API. is printed on Windows. Maybe the node version is too low.

@bajtos
Copy link
Member

bajtos commented Feb 4, 2019

Need to investigate why (node:1888) ExperimentalWarning: The http2 module is an experimental API. is printed on Windows. Maybe the node version is too low.

IIRC, AppVeyor is running an old version of Node.js 8.x. HTTP2 API was marked as stable at the time when 8.x was already in LTS state. We are also not able to upgrade to the latest version of Node.js 10.x because the tests are freezing - see #1431

@@ -20,7 +20,7 @@
"debug": "^4.0.1",
"fs-extra": "^7.0.0",
"glob": "^7.1.2",
"mocha": "^5.1.1",
"mocha": "^6.0.0-1",
Copy link
Member

Choose a reason for hiding this comment

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

As commented in the original PR, I am reluctant to use pre-release versions. Can we wait until 6.0.0 is released please, and then use that version instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. The PR was created to test out 6.x.

@@ -31,8 +31,8 @@ function run(argv, options) {
if (typeof options === 'boolean') options = {dryRun: options};
options = options || {};
if (setMochaOpts) {
Copy link
Member

@bajtos bajtos Feb 4, 2019

Choose a reason for hiding this comment

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

setMochaOpts is checking for presence of --opts. So IIUC the current implementation, if --opts is not provided, we add --config. Which means that lb-mocha --config my.json will add --config .mocharc.json to the command line, ending up with two --config flags. Is that desirable? I think it's not. Either way, please add a test to verify how lb-mocha behaves in this scenario.

How about backwards compatibility? If we replace --opts with --config in our check and stop detecting --opts, then I think this is a breaking change requiring a semver-major release. It may be more conventient for our users if we keep support for both --opts and --config options. Either way, please add a test to verify what happens when lb-mocha is invoked with --opts only: do we add --config or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's better to bump @loopback/build to the next major version. Using --opts for mocha@6.x seems to be add confusion.

Copy link
Member

Choose a reason for hiding this comment

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

I am fine with publishing a new semver-major version of @loopback/build. See https://github.com/strongloop/loopback-next/blob/master/docs/site/DEVELOPING.md#making-breaking-changes for a check-list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that mocha 6.x continues to support mocha.opts. I have improved the PR to respect --opts and mocha.opts in addition to .mocharc.* formats. It should be now backward compatible.

@raymondfeng raymondfeng changed the title [DO NOT MERGE] Upgrade to mocha v6.x chore(build): upgrade to mocha v6.x Feb 25, 2019
@bajtos
Copy link
Member

bajtos commented Feb 26, 2019

FWIW, mocha@6.0.0 is public: https://github.com/mochajs/mocha/releases/tag/v6.0.0

@bajtos
Copy link
Member

bajtos commented Feb 26, 2019

@raymondfeng what's the status of this pull request? How is it different from #2422 which you have already landed?

@raymondfeng
Copy link
Contributor Author

I would like to land this PR, which converts to the new --config and mocharc.json.

@raymondfeng raymondfeng changed the title chore(build): upgrade to mocha v6.x chore(build): use new config format for mocha v6.x Feb 26, 2019
@bajtos
Copy link
Member

bajtos commented Feb 28, 2019

I would like to land this PR

Sounds good to me. We need to follow the process for a new major version now.

Quoting from https://github.com/strongloop/loopback-next/blob/master/docs/site/DEVELOPING.md#making-breaking-changes

  • Describe incompatibilites for release notes
  • Look for more breaking changes to include in the release
  • Update list of supported versions

Most importantly, the commit message introducing the breaking change must include BREAKING CHANGE: footer - see https://github.com/strongloop/loopback-next/blob/master/docs/site/DEVELOPING.md#footer-optional.

We will need to update build's README to add a table listing supported major versions, I am ok to do that outside of this pull request if you prefer.

As for other breaking changes to include in the release, I am not really aware of any.

@bajtos
Copy link
Member

bajtos commented Feb 28, 2019

We will need to update build's README to add a table listing supported major versions.

Let's apply the same format we use in LB3 repositories, see e.g.
https://github.com/strongloop/loopback#module-long-term-support-policy

@raymondfeng raymondfeng force-pushed the boneskull-mocha-v6 branch 2 times, most recently from fe4ab2d to 71d7696 Compare March 5, 2019 16:27
@raymondfeng
Copy link
Contributor Author

@bajtos I think I have made it backward compatible except breaking changes from mocha 6.x itself.

'.mocharc.json',
'.mocharc.yaml',
'.mocharc.yml',
'mocha.opts',
Copy link
Member

@bajtos bajtos Mar 5, 2019

Choose a reason for hiding this comment

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

I believe the default config file is test/mocha.opts, not mocha.opts. Has this changed in mocha@6?

/cc @boneskull

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I fixed it.

@bajtos bajtos requested a review from a team March 5, 2019 16:39
@bajtos bajtos added Internal Tooling Issues related to our tooling and monorepo infrastructore developer-experience Issues affecting ease of use and overall experience of LB users labels Mar 5, 2019
@raymondfeng raymondfeng merged commit c3d8007 into master Mar 5, 2019
@raymondfeng raymondfeng deleted the boneskull-mocha-v6 branch March 5, 2019 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Issues affecting ease of use and overall experience of LB users Internal Tooling Issues related to our tooling and monorepo infrastructore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants