fix: make mocha self-contained with the source map support #913

raymondfeng · 2018-01-25T21:06:42Z

This is a follow-up to #884.
The commit makes it impossible to run npm test for individual packages.

There is no local mocha command for each package and it depends on the
global installation of mocha
With the global mocha, --require source-map-support/register cannot be
resolved.

The PR adds lb-mocha command so that we can configure source-map-support
correctly along with other options. No global mocha installation is needed.

Checklist

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

shimks · 2018-01-25T21:44:40Z

Can we converge back mocha.js.opts with mocha.ts.opts? They contain the same content and source-map-support stuff is done in run-mocha anyway

raymondfeng · 2018-01-25T22:53:36Z

@shimks I consolidated it back to mocha.opts.

bajtos

@raymondfeng This is awesome, I love how simpler and shorter our test-related package scripts are now 👍 And thank you for cleaning the mess I accidentally created with my previous pull request 🙇

I am afraid some of your changes are most likely breaking other things and need to be fixed:

single quotes don't work on Windows
I think lb-mocha is not available inside @loopback/build package

PTAL

bajtos · 2018-01-26T07:56:43Z

package.json

@@ -41,7 +39,7 @@
    "build:full": "npm run clean:full && npm run bootstrap && npm run build && npm run mocha && npm run lint",
    "pretest": "npm run clean && npm run build:current",
    "test": "node packages/build/bin/run-nyc npm run mocha",
-    "mocha": "node packages/build/bin/select-dist mocha --opts packages/build/mocha.ts.opts \"packages/*/DIST/test/**/*.js\" \"packages/cli/test/*.js\"",
+    "mocha": "node packages/build/bin/run-mocha --opts packages/build/mocha.opts \"packages/*/DIST/test/**/*.js\" \"packages/cli/test/*.js\"",


AFAICT, run-mocha is already adding --opts packages/build/mocha.opts, can you please remove --opts from this package script too?

bajtos · 2018-01-26T08:00:14Z

packages/build/README.md

-    "unit": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js'",
+    "acceptance": "lb-mocha --opts ../../test/mocha.opts 'DIST/test/acceptance/**/*.js'",
+    "integration": "lb-mocha --opts ../../test/mocha.opts 'DIST/test/integration/**/*.js'",
+    "test": "lb-mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js' 'DIST/test/integration/**/*.js' 'DIST/test/acceptance/**/*.js'",


Ditto. I believe lb-mocha is already adding --opts to CLI arguments.

Please make sure these scripts actually work when executed from packages/build directory. I would expect to see ./bin/run-mocha here, because it is my understanding that node_module/.bin/lb-mocha is created only when @loopback/build is installed as a (dev)dependency, which is not the case in @loopback/build itself.

Also --opts ../../test/mocha/opts looks suspicious to me, isn't this path pointing to monorepo-level test folder that no longer exists? I think we should not be needed --opts at all, since lb-mocha (run-mocha) is already adding them.

bajtos · 2018-01-26T08:02:07Z

packages/cli/generators/project/templates/package.json

@@ -35,7 +35,7 @@
 <% } -%>
    "pretest": "npm run clean && npm run build",
 <% if (project.mocha) { -%>
-    "test": "lb-dist mocha DIST/test",
+    "test": "lb-mocha 'DIST/test'",


AFAIK, single quotes don't work on Windows, that's why we have to use escaped double quotes in package scripts.

{ "test": "lb-mocha \"DIST/test\"" }

bajtos · 2018-01-26T08:05:06Z

packages/cli/package.json

@@ -54,7 +55,7 @@
  },
  "scripts": {
    "prepublishOnly": "nsp check",
-    "test": "mocha --opts node_modules/@loopback/build/mocha.js.opts \"test/*.js\""
+    "test": "lb-mocha 'test/*.js'"


Ditto, please preserve original double quotes here and also in all other package scripts you have modified in this patch.

See https://stackoverflow.com/q/10737283/69868 and https://stackoverflow.com/a/24181667/69868

bajtos · 2018-01-26T08:06:03Z

packages/authentication/package.json

@@ -6,18 +6,18 @@
    "node": ">=6"
  },
  "scripts": {
-    "acceptance": "lb-dist mocha --opts node_modules/@loopback/build/mocha.ts.opts 'DIST/test/acceptance/**/*.js'",
+    "acceptance": "lb-mocha 'DIST/test/acceptance/**/*.js'",


Please change single quotes to double quotes while you are changing test-related scripts in this file. (See my other comments below for more details.)

bajtos · 2018-01-26T08:11:22Z

One more comment: please update packages/build/README.md to reflect the changes you are making here. The current content does not look correct to me:

    "acceptance": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/acceptance/**/*.js'",
    "integration": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/integration/**/*.js'",
    "test": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js' 'DIST/test/integration/**/*.js' 'DIST/test/acceptance/**/*.js'",
    "unit": "lb-dist mocha --opts ../../test/mocha.opts 'DIST/test/unit/**/*.js'",

bajtos · 2018-01-26T08:12:47Z

packages/build/bin/run-mocha.js

+
+  // Substitute the DIST variable with the dist folder
+  const dist = utils.getDistribution();
+  const mochaOpts = argv.slice(2).map(a => a.replace(/\bDIST\b/g, dist));


Since this has effectively inlined select-dist script, I think we can remove select-dist entirely? Just make sure to mention that removal in the commit message as a BREAKING CHANGE.

We have to leave lb-dist there to keep backward compatibility as projects generated from previous versions of our CLI reference lb-dist.

Makes sense.

To me, this is another argument against using.alpha.X prereleases for so long. If we were versioning non-core modules as 0.x.y, then we could treat semver-patch release as backwards compatible and semver-minor releases as breaking. This works great with the carrot operator as implemented by npm's semver (https://www.npmjs.com/package/semver#caret-ranges-123-025-004):

this allows patch and minor updates for versions 1.0.0 and above, patch updates for versions 0.X >=0.1.0, and no updates for versions 0.0.X.

Agree with the versioning. Is it too late to fix it?

Agree with the versioning.

👍

Is it too late to fix it?

I don't know. Let's open a new issue and try to find out what the implications of such change would be & how much work this change would require:

#954

bajtos · 2018-01-26T17:00:29Z

@raymondfeng when you rebase on top of recently landed #908, please update package.json in example-log-extension too.

shimks · 2018-01-26T17:18:39Z

When I run npm t on the monorepo, the tests in the cli/test/sandbox/test also seem to be running. Could you also fix that with this PR please?

This is a follow-up to #884. The changes make it impossible to run 'npm test' for individual packages. - There is no local mocha command for each package and it depends on the global installation of 'mocha' - With the global mocha, --require source-map-support/register cannot be resolved. The PR adds 'lb-mocha' command so that we can support source-map-support correctly along with other options. No global mocha installation is needed.

raymondfeng · 2018-01-26T17:47:21Z

PTAL

raymondfeng · 2018-01-26T17:49:29Z

single quotes don't work on Windows

Fixed with \".

I think lb-mocha is not available inside @loopback/build package

That's expected. That's why we only use lb-mocha for modules that have @loopback/build as a dependency.

bajtos · 2018-01-29T10:32:37Z

packages/metadata/test/unit/inspector.test.ts

@@ -631,6 +633,15 @@ describe('Inspector for design time metadata', () => {
    expect(meta).to.eql(MyClass);
  });

+  it('inspects design time type for properties with union type', () => {


This looks like unrelated changes to me. @raymondfeng could you please check these changes were intended?

raymondfeng requested review from bajtos and kjdelisle as code owners January 25, 2018 21:06

raymondfeng added the needs-priority label Jan 25, 2018

shimks mentioned this pull request Jan 25, 2018

fix(example-getting-started): remove api-docs #912

Closed

6 tasks

raymondfeng force-pushed the fix-mocha-source-map branch from 4b66ead to 56fc63b Compare January 25, 2018 21:23

virkt25 approved these changes Jan 25, 2018

View reviewed changes

raymondfeng force-pushed the fix-mocha-source-map branch from 56fc63b to cc2333c Compare January 25, 2018 22:51

raymondfeng requested a review from shimks as a code owner January 25, 2018 22:51

bajtos suggested changes Jan 26, 2018

View reviewed changes

bajtos reviewed Jan 26, 2018

View reviewed changes

raymondfeng force-pushed the fix-mocha-source-map branch from cc2333c to 7bc8125 Compare January 26, 2018 17:47

raymondfeng requested a review from jannyHou as a code owner January 26, 2018 17:47

b-admike approved these changes Jan 26, 2018

View reviewed changes

jannyHou approved these changes Jan 26, 2018

View reviewed changes

kjdelisle approved these changes Jan 26, 2018

View reviewed changes

raymondfeng merged commit 7c6d869 into master Jan 26, 2018

raymondfeng removed the needs-priority label Jan 26, 2018

bajtos deleted the fix-mocha-source-map branch January 29, 2018 10:25

bajtos reviewed Jan 29, 2018

View reviewed changes

bajtos mentioned this pull request Feb 2, 2018

Switch to 0.x.y versions instead of 4.0.0-alpha.X #954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make mocha self-contained with the source map support #913

fix: make mocha self-contained with the source map support #913

raymondfeng commented Jan 25, 2018 •

edited

Loading

shimks commented Jan 25, 2018 •

edited

Loading

raymondfeng commented Jan 25, 2018

bajtos left a comment

bajtos Jan 26, 2018

bajtos Jan 26, 2018 •

edited

Loading

bajtos Jan 26, 2018

bajtos Jan 26, 2018

bajtos Jan 26, 2018

bajtos Jan 26, 2018

bajtos commented Jan 26, 2018

bajtos Jan 26, 2018

raymondfeng Jan 26, 2018

bajtos Jan 29, 2018

raymondfeng Jan 29, 2018

bajtos Feb 2, 2018

bajtos commented Jan 26, 2018

shimks commented Jan 26, 2018

raymondfeng commented Jan 26, 2018

raymondfeng commented Jan 26, 2018

bajtos Jan 29, 2018

fix: make mocha self-contained with the source map support #913

fix: make mocha self-contained with the source map support #913

Conversation

raymondfeng commented Jan 25, 2018 • edited Loading

Checklist

shimks commented Jan 25, 2018 • edited Loading

raymondfeng commented Jan 25, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Jan 26, 2018

shimks commented Jan 26, 2018

raymondfeng commented Jan 26, 2018

raymondfeng commented Jan 26, 2018

Choose a reason for hiding this comment

raymondfeng commented Jan 25, 2018 •

edited

Loading

shimks commented Jan 25, 2018 •

edited

Loading

bajtos Jan 26, 2018 •

edited

Loading