[INTERNAL] test: improved test coverage #91

romaniam · 2018-12-13T13:15:25Z

Added tests for build command handler

CLAassistant · 2018-12-13T13:15:33Z

All committers have signed the CLA.

CLAassistant · 2018-12-13T13:15:33Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

coveralls · 2018-12-13T13:37:45Z

Coverage increased (+42.4%) to 82.117% when pulling 1a78601 on increasing-test-coverage into ba6f368 on master.

…mand module.

romaniam · 2018-12-13T17:52:27Z

In some files I had to add test.serial, otherwise the tests fail because the stub references are shared... This is a known issue with ava and sinon stackoverflow: Problems with ava async tests when stubbing with sinon

… (required for testing)

…eve the key from a specific path

…t values

… base and logger middleware

matz3 · 2018-12-19T16:14:22Z

lib/cli/commands/init.js

@@ -1,44 +1,29 @@
+const validate = require("../../utils/validate");


We move the require statements into the handler function to not load the dependencies all the time.
This had a significant impact e.g. when just calling ui5 --version as all command files need to be required, but not all code in there needs to be executed.

A better approach might be to move the handler functions in a separate module that is then required from the yargs handler function once it's called.

Good points, thanks. Personally I would prefer not to move the handler to a separate module. How about this approach: 29fc902 ? (just saw the async flag which is not required, will remove it after your next review)

RandomByte

Didn't review the tests yet as I would like to clarify the mocking of require first.

RandomByte · 2019-01-02T13:13:18Z

lib/cli/commands/init.js

-			}
-		}
-	}
+initCommand.lazyRequireDependencies = async function() {


What about using mock-require here instead of adding this function to the productive code just for resting purposes?

I tried to avoid adding a new dependency with a module which was maintained 9 months ago and "only" 300 stars + mocking module loaders is not really a good solution e.g. switching to ES6 modules will not be so easy anymore...

As discussed, module-require is now used to mock module loading

RandomByte · 2019-01-02T13:17:23Z

lib/cli/commands/serve.js

@@ -46,8 +46,12 @@ serve.builder = function(cli) {
 			"Listen to port 1337 and launch default browser with http://localhost:1337/test/QUnit.html");
 };

-serve.handler = function(argv) {
+serve.openUrl = async function(browserUrl) {


Same here, adding a function just to make testing easier should be avoided if possible. What about mock-require?

The function is called just one time, so no real need to have it in a separate function.

How should we test the functionality of the url opening then? e.g. spy on opn call will not work.

the method adds more flexibility in my opinion e.g. adding validation if browser, easier to test, easier to maintain (e.g. dependency replacement)

RandomByte · 2019-01-02T13:20:05Z

lib/cli/commands/serve.js

 		};
+		return !serverConfig.h2
+			? {serverConfig, tree}
+			: ui5Server.sslUtil.getSslCertificate(serverConfig.key, serverConfig.cert).then(({key, cert}) => {


This is really hard to read. Please use if/else

Yeah sorry about that. Is fixed with in c2b125f

RandomByte · 2019-01-02T13:42:01Z

lib/utils/validate.js

@@ -0,0 +1,43 @@
+const {promisify} = require("util");


Could we rename this file to fsHelper or something alike?

In my opinion it doesn't really "validate" anything or throws errors if some validation failed. It currently just offers convenience wrappers for some fs APIs.

Good point. renamed as requested c2b125f

RandomByte · 2019-01-02T13:45:41Z

lib/cli/middlewares/base.js

-			logger.getLogger("cli:middlewares:base").verbose(`using node version ${process.version}`);
-		}
-	}
+module.exports = function(argv = {}) {


Why default argv to an empty object just to return null if argv is an empty object?

The default param is there to avoid errors when properties are accessed...

RandomByte · 2019-01-02T13:46:18Z

lib/cli/middlewares/logger.js

+ * @param {Object} argv logger arguments
+ * @returns {Object} logger instance or null
+ */
+function init(argv = {}) {


argv is not optional, why defaulting it then?

The default param is there to avoid errors when properties are accessed...

RandomByte · 2019-01-02T13:48:18Z

lib/cli/commands/serve.js

+	return opn(browserUrl);
+};
+
+serve.handler = function(argv = {}) {


Why the defaulting to empty object? argv should always be supplied shouldn't it?

The default param is there to avoid errors when properties are accessed e.g.

normalizer.generateProjectTree({ translator: argv.translator, configPath: argv.config })

matz3 · 2019-01-03T08:58:40Z

test/lib/cli/commands/build.js

+	await build.handler(args);
+	t.deepEqual(
+		builderStub.getCall(0).args[0],
+		Object.assign({}, {dev: true}, defaultBuilderArgs),


Doesn't this still result into dev: false as the defaultBuilderArgs are passed as last arg?
Assignment is from right to left.

It seems that the fact that args and the stubs are shared across tests and they are executed in parallel, this seems to somehow work.

Done with 30dd0b7

… base and logger middleware

…-cli into increasing-test-coverage

RandomByte · 2019-01-07T09:37:05Z

lib/cli/commands/init.js

 	command: "init",
 	describe: "Initialize the UI5 Build and Development Tooling configuration for an application or library project.",
 	middlewares: [require("../middlewares/base.js")]
 };

-init.handler = async function(argv) {
+initCommand.lazyRequireDependencies = async function() {


What is the point of having this lazyRequireDependencie function still in the code?

You are right, it's totally useless. Removed...

RandomByte · 2019-01-07T12:41:24Z

test/lib/cli/commands/build.js

 	normalizerStub = sinon.stub(normalizer, "generateProjectTree");
 	builderStub = sinon.stub(builder, "build").returns(Promise.resolve());
 	loggerStub = sinon.stub(logger, "setShowProgress");
 });

-test.after("Restore mocks after test exection", (t) => {
+test.afterEach("Restore mocks after test exection", (t) => {
 	loggerStub.restore();


We could also use sinon.restore() in a test.afterEach.always callback as described here:
https://sinonjs.org/releases/v7.2.2/general-setup/

But we can also change that in a follow up PR.

Great, I like it 👍
Fixed in b282427

I think it would be even better to use test.afterEach.always. That way, even if a test fails, the stubs created in that test are reset and can't influence any other tests.

Also, I'm not sure whether you need to supply a message to the beforeEach and afterEach hooks. I couldn't find this mentioned anywhere in the ava docs.

test/lib/cli/commands/build.js

test/lib/cli/commands/init.js

romaniam

Resolved suggested fixes. Please re-review

RandomByte · 2019-01-08T10:00:40Z

test/lib/cli/commands/build.js

 	normalizerStub = sinon.stub(normalizer, "generateProjectTree");
 	builderStub = sinon.stub(builder, "build").returns(Promise.resolve());
 	loggerStub = sinon.stub(logger, "setShowProgress");
 });

-test.after("Restore mocks after test exection", (t) => {
+test.afterEach("Restore mocks after test exection", (t) => {
 	loggerStub.restore();


I think it would be even better to use test.afterEach.always. That way, even if a test fails, the stubs created in that test are reset and can't influence any other tests.

Also, I'm not sure whether you need to supply a message to the beforeEach and afterEach hooks. I couldn't find this mentioned anywhere in the ava docs.

RandomByte · 2019-01-08T16:57:20Z

Ok from my side but we need to make sure to squash all commits while merging (but not before!).

Also remove messages from hooks. They don't seem to have any effect.

Obsolete

[INTERNAL] build: added tests. Slightly refactored build handler method.

ff42d8e

romaniam requested a review from RandomByte December 13, 2018 13:15

[FIX] dependencies: Added sinon as dependency

4d10beb

[INTERNAL] serve: added some tests. Slightly refactored the serve com…

f96d4f9

…mand module.

romaniam added 7 commits December 13, 2018 19:29

[INTERNAL] serve: moved the opn module initialisation into a function…

9849ca1

… (required for testing)

[INTERNAL] serve: Added additional options tests e.g. open

38c2333

[FIX] serve: key and cert are now passed to getSsslCert func to retri…

f32bd87

…eve the key from a specific path

[FIX] serve: fixed failing tests because of added cert and key defaul…

3105a90

…t values

[INTERNAL] serve: added tests for cert and key options

32d2a0d

[INTERNAL] serve: added tests for config and translate options

f82fd31

[INTERNAL] init: added tests

b266457

RandomByte mentioned this pull request Dec 19, 2018

[Roadmap] Production ready UI5 CLI SAP/ui5-tooling#92

Closed

9 tasks

romaniam added 5 commits December 19, 2018 12:23

[INTERNAL] increased code coverage and refactored some modules

902af47

[INTERNAL] test: added tests for tree command

a508006

[INTERNAL] test: added tests for base middleware

a2785bf

[INTERNAL] clean + test: refactored middleware, implemented tests for…

4fc780e

… base and logger middleware

[INTERNAL] clean + test: refactored versions. added versions tests

b4fef79

romaniam changed the title ~~[INTERNAL] build: added tests. Slightly refactored build handler method.~~ [INTERNAL] test: improved test coverage Dec 19, 2018

romaniam requested a review from matz3 December 19, 2018 15:22

matz3 previously requested changes Dec 21, 2018

View reviewed changes

[INTERNAL] Added module lazy loading for init command

29fc902

RandomByte requested changes Jan 2, 2019

View reviewed changes

[INTERNAL] Refactored a few modules

c2b125f

matz3 reviewed Jan 3, 2019

View reviewed changes

matz3 and others added 2 commits January 3, 2019 10:02

Fix cli/commands/build.js tests

30dd0b7

Removed default params

e129981

romaniam and others added 14 commits January 7, 2019 08:33

[INTERNAL] serve: added tests for config and translate options

6963558

[INTERNAL] init: added tests

0832adc

[INTERNAL] increased code coverage and refactored some modules

93db6fe

[INTERNAL] test: added tests for tree command

2cf9b15

[INTERNAL] test: added tests for base middleware

b4c6ddd

[INTERNAL] clean + test: refactored middleware, implemented tests for…

43055dc

… base and logger middleware

[INTERNAL] clean + test: refactored versions. added versions tests

95accb3

[INTERNAL] Added module lazy loading for init command

5072c11

[INTERNAL] Refactored a few modules

41abd8c

Fix cli/commands/build.js tests

feafead

Removed default params

b3eec89

Added mock require. Added mock for opn module in serve tests

10b711a

[INTERNAL] Integrated mock-require for commands tests

a3b97cc

Merge branch 'increasing-test-coverage' of https://github.com/SAP/ui5…

014d995

…-cli into increasing-test-coverage

RandomByte requested changes Jan 7, 2019

View reviewed changes

romaniam force-pushed the increasing-test-coverage branch from 5957e91 to b282427 Compare January 8, 2019 08:39

romaniam commented Jan 8, 2019

View reviewed changes

RandomByte requested changes Jan 8, 2019

View reviewed changes

[INTERNAL] Replaced mock require with sinon stubs

bdc66e1

romaniam force-pushed the increasing-test-coverage branch from 18fce64 to ef6289c Compare January 8, 2019 16:08

romaniam and others added 3 commits January 9, 2019 16:19

[INTERNAL] Removed unused function

d0a4b38

[INTERNAL] All stubs are now restored using sinon.restore()

bb3781b

[INTERNAL] Tests: Restore sutubs in test.afterEach.always hook

0824ad1

Also remove messages from hooks. They don't seem to have any effect.

romaniam force-pushed the increasing-test-coverage branch from 514d07e to 0824ad1 Compare January 9, 2019 15:21

Merge branch 'master' into increasing-test-coverage

1a78601

RandomByte approved these changes Jan 9, 2019

View reviewed changes

romaniam merged commit c6893d0 into master Jan 9, 2019

romaniam deleted the increasing-test-coverage branch January 9, 2019 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INTERNAL] test: improved test coverage #91

[INTERNAL] test: improved test coverage #91

romaniam commented Dec 13, 2018

CLAassistant commented Dec 13, 2018 •

edited

Loading

CLAassistant commented Dec 13, 2018

coveralls commented Dec 13, 2018 •

edited

Loading

romaniam commented Dec 13, 2018 •

edited

Loading

matz3 Dec 19, 2018

romaniam Dec 21, 2018

RandomByte left a comment

RandomByte Jan 2, 2019

romaniam Jan 3, 2019

romaniam Jan 7, 2019

RandomByte Jan 2, 2019

romaniam Jan 3, 2019

RandomByte Jan 2, 2019

romaniam Jan 3, 2019

RandomByte Jan 2, 2019

romaniam Jan 3, 2019

RandomByte Jan 2, 2019

romaniam Jan 3, 2019

RandomByte Jan 2, 2019

romaniam Jan 3, 2019

RandomByte Jan 2, 2019

romaniam Jan 3, 2019

matz3 Jan 3, 2019

matz3 Jan 3, 2019

RandomByte Jan 7, 2019

romaniam Jan 8, 2019

RandomByte Jan 7, 2019

romaniam Jan 8, 2019

RandomByte Jan 8, 2019

romaniam left a comment •

edited

Loading

RandomByte Jan 8, 2019

RandomByte commented Jan 8, 2019

		@@ -1,44 +1,29 @@
		const validate = require("../../utils/validate");

[INTERNAL] test: improved test coverage #91

[INTERNAL] test: improved test coverage #91

Conversation

romaniam commented Dec 13, 2018

CLAassistant commented Dec 13, 2018 • edited Loading

CLAassistant commented Dec 13, 2018

coveralls commented Dec 13, 2018 • edited Loading

romaniam commented Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte left a 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

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

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

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

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

romaniam left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RandomByte commented Jan 8, 2019

CLAassistant commented Dec 13, 2018 •

edited

Loading

coveralls commented Dec 13, 2018 •

edited

Loading

romaniam commented Dec 13, 2018 •

edited

Loading

romaniam left a comment •

edited

Loading