test: run cli tests in top level npm test #825

virkt25 · 2018-01-02T15:33:44Z

Description

CLI Tests aren't run as part of top level npm test. Test count before change was 609. After this change it is 694.

Related issues

connect to <link_to_referenced_issue>

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style guide

bajtos · 2018-01-02T15:36:36Z

I am fine with this change as a short-term fix, but wouldn't it better to fix the layout of CLI test folder to follow the convention we have everywhere else, namely "unit", "integration" and "acceptance" folders?

kjdelisle

If the CLI tests aren't following our monorepo layout, that tells me something fishy is going on...

kjdelisle · 2018-01-02T15:53:41Z

package.json

@@ -39,7 +39,7 @@
    "build:current": "lerna run --loglevel=silent build:current",
    "pretest": "npm run build:current",
    "test": "node packages/build/bin/run-nyc npm run mocha",
-    "mocha": "node packages/build/bin/select-dist mocha --opts test/mocha.opts \"packages/*/DIST/test/**/*.js\"",
+    "mocha": "node packages/build/bin/select-dist mocha --opts test/mocha.opts \"packages/*/DIST/test/**/*.js\" \"packages/cli/test\"",


Why is there no dist folder for the CLI tests? It should follow the existing pattern in the current command (packages/cli/dist/test/**/*.js)

@kjdelisle I think the CLI cannot use TypeScript because it's a Yeoman generator under the hood, which has implications on the project layout. One more reason for getting rid of Yeoman! See #844

virkt25 · 2018-01-02T16:05:01Z

CLI is written in JS not TS! So there's no build for it, which means there is no dist folder for it.

raymondfeng · 2018-01-02T16:50:58Z

@virkt25 Please check why CI fails.

raymondfeng · 2018-01-02T19:14:34Z

package.json

@@ -37,7 +37,8 @@
    "clean": "lerna run --loglevel=silent clean",
    "build": "lerna run --loglevel=silent build",
    "build:current": "lerna run --loglevel=silent build:current",
-    "pretest": "npm run build:current",
+    "pretest": "npm run build:current && npm run cli:link",
+    "cli:link": "cd packages/cli && npm link",


Why do we do this?

I'm okay with the cli:link command, but not having it added to pretest. This means that any time someone runs tests, they're going to lose their existing global installation of the CLI and have it replaced with the locally-linked one. I expect this would cause havoc for contributors down the line.

kjdelisle

Remove the alteration to pretest, IMO.

kjdelisle · 2018-01-02T19:31:23Z

package.json

@@ -37,7 +37,8 @@
    "clean": "lerna run --loglevel=silent clean",
    "build": "lerna run --loglevel=silent build",
    "build:current": "lerna run --loglevel=silent build:current",
-    "pretest": "npm run build:current",
+    "pretest": "npm run build:current && npm run cli:link",
+    "cli:link": "cd packages/cli && npm link",


I'm okay with the cli:link command, but not having it added to pretest. This means that any time someone runs tests, they're going to lose their existing global installation of the CLI and have it replaced with the locally-linked one. I expect this would cause havoc for contributors down the line.

virkt25 · 2018-01-02T20:04:04Z

I'm testing this right now to see if this fixes the issue with tests failing on Windows based on http://yeoman.io/authoring/

I'll ping the team if it works and is ready for review.

raymondfeng · 2018-01-02T21:41:06Z

package.json

@@ -38,7 +38,7 @@
    "build": "lerna run --loglevel=silent build",
    "build:current": "lerna run --loglevel=silent build:current",
    "pretest": "npm run build:current && npm run cli:link",
-    "cli:link": "cd packages/cli/generators && npm link && cd ../../..",
+    "cli:link": "cd packages/cli/generators/app && npm link && cd ../controller && npm link && ../extension && npm link && ../project && npm link && cd ../../..",


This is completely wrong. Can you explain?

Please note we don't depend on global installation of generators as @loopback/cli does not use yo.

I see. I'll have to see if I can get my hands on a windows machine as I see a lot of tests failing on AppVeyor with the following message:

Error: You don't seem to have a generator with the name “loopback4:C:\projects\loopback-next\packages\cli\generators\app” installed. But help is on the way: You can see available generators via npm search yeoman-generator or via http://yeoman.io/generators/. Install them with npm install generator-loopback4:C:\projects\loopback-next\packages\cli\generators\app. To see all your installed generators run yo without any arguments. Adding the --help option will also show subgenerators. If yo cannot find the generator, run yo doctor to troubleshoot your system. at Environment.create (packages\cli\node_modules\yeoman-environment\lib\environment.js:368:9) at Object.exports.testSetUpGen (packages\cli\test\test-utils.js:15:14) at Context.it (packages\cli\test\base-generator.js:15:29)

raymondfeng · 2018-01-02T22:17:13Z

I think I know what causes the Windows build issue now. There are a few places that we use / as a separator to split paths.

We need to use path.sep as the separator so that it works on all platforms.

* test: run cli tests in top level npm test * fix(cli): use path.sep to be platform agnostic

virkt25 requested review from bajtos, kjdelisle and raymondfeng as code owners January 2, 2018 15:33

virkt25 added the needs-priority label Jan 2, 2018

kjdelisle suggested changes Jan 2, 2018

View reviewed changes

kjdelisle approved these changes Jan 2, 2018

View reviewed changes

raymondfeng reviewed Jan 2, 2018

View reviewed changes

kjdelisle suggested changes Jan 2, 2018

View reviewed changes

virkt25 changed the title ~~test: run cli tests in top level npm test~~ [WIP] test: run cli tests in top level npm test Jan 2, 2018

virkt25 force-pushed the cli-tests branch from 8ba4478 to 4f2e4b2 Compare January 2, 2018 20:35

raymondfeng reviewed Jan 2, 2018

View reviewed changes

virkt25 added 2 commits January 2, 2018 18:28

test: run cli tests in top level npm test

3ce0339

fix(cli): use path.sep to be platform agnostic

ee9fe83

virkt25 force-pushed the cli-tests branch from b1c3f70 to ee9fe83 Compare January 2, 2018 23:28

virkt25 changed the title ~~[WIP] test: run cli tests in top level npm test~~ test: run cli tests in top level npm test Jan 2, 2018

raymondfeng approved these changes Jan 2, 2018

View reviewed changes

virkt25 merged commit 6474763 into master Jan 3, 2018

virkt25 deleted the cli-tests branch January 3, 2018 00:16

virkt25 removed the needs-priority label Jan 3, 2018

virkt25 added a commit that referenced this pull request Jan 3, 2018

test: run cli tests in top level npm test (#825)

646d63e

* test: run cli tests in top level npm test * fix(cli): use path.sep to be platform agnostic

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: run cli tests in top level npm test #825

test: run cli tests in top level npm test #825

virkt25 commented Jan 2, 2018 •

edited

Loading

bajtos commented Jan 2, 2018

kjdelisle left a comment

kjdelisle Jan 2, 2018

bajtos Jan 5, 2018

virkt25 commented Jan 2, 2018

raymondfeng commented Jan 2, 2018

raymondfeng Jan 2, 2018

kjdelisle Jan 2, 2018 •

edited

Loading

kjdelisle left a comment

kjdelisle Jan 2, 2018 •

edited

Loading

virkt25 commented Jan 2, 2018

raymondfeng Jan 2, 2018

raymondfeng Jan 2, 2018

virkt25 Jan 2, 2018

raymondfeng commented Jan 2, 2018

test: run cli tests in top level npm test #825

test: run cli tests in top level npm test #825

Conversation

virkt25 commented Jan 2, 2018 • edited Loading

Description

Related issues

Checklist

bajtos commented Jan 2, 2018

kjdelisle left a comment

Choose a reason for hiding this comment

kjdelisle Jan 2, 2018

Choose a reason for hiding this comment

bajtos Jan 5, 2018

Choose a reason for hiding this comment

virkt25 commented Jan 2, 2018

raymondfeng commented Jan 2, 2018

raymondfeng Jan 2, 2018

Choose a reason for hiding this comment

kjdelisle Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

kjdelisle left a comment

Choose a reason for hiding this comment

kjdelisle Jan 2, 2018 • edited Loading

Choose a reason for hiding this comment

virkt25 commented Jan 2, 2018

raymondfeng Jan 2, 2018

Choose a reason for hiding this comment

raymondfeng Jan 2, 2018

Choose a reason for hiding this comment

virkt25 Jan 2, 2018

Choose a reason for hiding this comment

raymondfeng commented Jan 2, 2018

virkt25 commented Jan 2, 2018 •

edited

Loading

kjdelisle Jan 2, 2018 •

edited

Loading

kjdelisle Jan 2, 2018 •

edited

Loading