Update Testing your application #635

shimks · 2018-02-23T19:06:41Z

A question for anyone that's familiar with test databases: should I be creating a separate config for test-specific databases? If so, where should the config file and the typescript file live and how should they be named?

please review the first commit since the commit after introduces line breaks (403489f)

b-admike · 2018-02-23T19:20:14Z

pages/en/lb4/Testing-your-application.md

-<!--
-If you are just getting started with LoopBack, use the LoopBack command-line tool (CLI)  `loopback-cli`. It's ready to use and installs simply by `npm install -g loopback-cli`. You don't need to do any extra steps for setup, and can head straight to the [next section](Testing-Your-Application#acceptance-testing.html).
-->
+LoopBack applications that have been generated by `lb4 app` or `lb4`


You can add here that if the user selects all our add-ons such as tslint and mocha during app creation, then they get them added as deps too with configs.

kjdelisle

Ugh. I just spent a decent amount of time reviewing this only to realize that most of what I'm proposing changes to is not actually a part of the changeset.

This is why I would prefer that we have 80 chars per line whenever possible; GitHub's diff wrapping seems to expect it!

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

pages/en/lb4/Testing-your-application.md


-{% include important.html content="A great test suite requires you to think smaller and favor fast, focused unit tests over slow application-wide end-to-end tests
+{% include important.html content="
+A great test suite requires you to think smaller and favor fast, focused unit tests over slow application-wide end-to-end tests.


over slow end-to-end tests

Alternatively,

over slow, application-wide, end-to-end test

I would prefer the first, since end-to-end already implies that it covers the whole application, otherwise how could it be "end-to-end"?

kjdelisle · 2018-02-23T19:57:51Z

pages/en/lb4/Testing-your-application.md

@@ -331,13 +330,13 @@ describe('Person (unit)', () => {
 });
 ```

-Writing a unit test for a custom repository methods is not straightforward because `CrudRepositoryImpl` is based on legacy loopback-datasource-juggler that was not designed with dependency injection in mind. Instead, use integration tests to verify the implementation of custom repository methods; see [Test your repositories against a real database](#test-your-repositories-against-a-real-database) in [Integration Testing](#integration-testing).
+Writing a unit test for a custom repository methods is not as straightforward because `CrudRepository` is based on legacy [loopback-datasource-juggler](https://github.com/strongloop/loopback-datasource-juggler) that was not designed with dependency injection in mind. Instead, use integration tests to verify the implementation of custom repository methods; see [Test your repositories against a real database](#test-your-repositories-against-a-real-database) in [Integration Testing](#integration-testing).


Replace ...that was not designed with...
with
...which was not designed with...

kjdelisle · 2018-02-23T20:26:52Z

pages/en/lb4/Testing-your-application.md


 ### Unit test your Sequence

-While it's possible to test a custom Sequence class in isolation, it's better to rely on acceptance-level tests in this exceptional case. The reason is that a custom Sequence class typically has many dependencies (which makes test setup too long and complex), and at the same time it provides very little functionality on top of the injected sequence actions. Bugs are much more likely to caused by the way how the real sequence action implementations interact together (which is not covered by unit tests), instead of the Sequence code itself (which is the only thing covered).
+While it's possible to test a custom Sequence class in isolation, it's better to rely on acceptance-level tests in this exceptional case. The reason is that a custom Sequence class typically has many dependencies (which makes test setup too long and complex), and at the same time it provides very little functionality on top of the injected sequence actions. Bugs are much more likely to be caused by the way the real sequence action implementations interact together (which is not covered by unit tests), instead of the Sequence code itself (which is the only thing covered).


which makes test setup too long and complex -> which can make test setup too long or complex

shimks · 2018-02-28T22:02:03Z

@bajtos Are we still advocating for attaching util-like functions to the model itself as seen in here? http://loopback.io/doc/en/lb4/Testing-your-application.html#unit-test-your-models-and-repositories

bajtos · 2018-03-01T13:59:32Z

This is why I would prefer that we have 80 chars per line whenever possible; GitHub's diff wrapping seems to expect it!

I had the impression that GitHub's markdown renderer preserves line breaks, which is something we don't want 😞Because I could not find any documentation for this, I set out to find out the real behavior myself.

TL;DR: GitHub treats newlines as soft line breaks (i.e. a breakable space) when rendering markdown files (which is great!), but treats newlines as hard line breaks when rendering issue/pull request descriptions and possibly comments too (which I find confusing and often annoying).

https://github.github.com/gfm/#hard-line-breaks

A line break (not in a code span or HTML tag) that is preceded by two or more spaces and does not occur at the end of a block is parsed as a hard line break (rendered in HTML as a
tag).

For a more visible alternative, a backslash before the line ending may be used instead of two spaces.

https://github.github.com/gfm/#soft-line-breaks

A regular line break (not in a code span or HTML tag) that is not preceded by two or more spaces or a backslash is parsed as a softbreak. (A softbreak may be rendered in HTML either as a line ending or as a space. The result will be the same in browsers. In the examples here, a line ending will be used.)

A simple Markdown document to verify this behavior can be found here:

So! Based on this investigation, I fully support your push for 80-char-wide lines in Markdown documents now, @kjdelisle.

bschrammIBM

line 145 If this line is going to remain in the final version, then you need to remove the initials. "shmks" Also I fixed the english a little. (which have not been)
+

minor edit in line 474: (Use "it" not "they")
Therefore, the code outlined in this section is outdated and may not work out of the box. It will be revisited after our MVP release.

line 480 - need to "a" - (a good thing)
Extending your API spec with examples is a good thing on its own, since developers consuming your API will find them useful too.

virkt25 · 2018-03-05T17:18:06Z

pages/en/lb4/Testing-your-application.md

-<!--
-If you are just getting started with LoopBack, use the LoopBack command-line tool (CLI)  `loopback-cli`. It's ready to use and installs simply by `npm install -g loopback-cli`. You don't need to do any extra steps for setup, and can head straight to the [next section](Testing-Your-Application#acceptance-testing.html).
-->
+LoopBack applications that have been generated by `lb4 app` or `lb4`


LoopBack application that have been generated using the lb4 app command from @loopback/cli come with @loopback/testlab and mocha as a default, so no other testing infrastructure setup is needed.

^ I tried to make it shorter and more to the point. Don't think we need to list both cli commands, just the one we promote as a default for creating a new project in examples. And don't think we need to mention it's a prompt. They should know that because they would've already created the app by now.

virkt25 · 2018-03-05T17:20:49Z

pages/en/lb4/Testing-your-application.md

    "mocha": "^<current-version>"
  },
  "scripts": {
-    "test": "mocha"
+    "test": "mocha --recursive \"DIST/test\""


Use of DIST requires the use of lb-mocha from @loopback/build which replaces DIST with the appropriate folder name based on version ... If you want to use mocha then it should be dist since we no longer support Node 6 (dist6).

jannyHou

Still reviewing more code, just some personal opinion for your question regarding database:

The memory db created in example-getting-started is easy to setup and stores your data in the specified file, which IMO is a reasonable choice for testing data persistence.

bschrammIBM

Line 57: need to make application plural!
LoopBack applications that have been generated using the lb4 app command from

132-135: suggest the following rewrite:
Easier to understand, since it's immediately clear what model properties are
+relevant to the tests. If the tests set the required properties,
+it is difficult to tell whether the properties are actually
+relevant to the tested scenario.

lines 138-141, suggest the following rewrite:
If the tests build model instance data
+manually, you would have to update all tests to set a new required property.
+With a shared helper, you update a single location with the new property.

jannyHou · 2018-03-05T20:31:13Z

pages/en/lb4/Testing-your-application.md


-      const details = await controller.getDetails(1);
+      const details = await controller.getDetails('pen');


@shimks A general question: do we have an example package contains all these code?
I came across the thought when searched for function getDetails, it should be defined in some folded snippet. Just feel if we have an example testing package might be more straight-forward.(out of the scope of your PR definitely)

I've actually went ahead and created an app to make sure these codes work. These code snippets are literally code snippets from the project that I've made :)

bschrammIBM

lines 254-256: control not controls
+Unit tests are considered "white-box" tests because they use an "inside-out"
+approach where the tests know about the internals and control all the variables
+of the system being tested.

shimks · 2018-03-05T20:32:40Z

@jannyHou then the question becomes should the memory db exist in src folder since it'll be solely for the test? Or do we want to have it also be the production database?

bschrammIBM · 2018-03-05T20:36:11Z

I am copyediting this section. I will need to finish it tomorrow as we have a translation drop on Weds for Velox. I have submitted a few copyedits. I did want to make a comment that the section is very long and reads like a research paper. I am thinking it should be trimmed to provide only the suggested steps for generating LB4 tests, rather than providing so much discussion? Or maybe the intent was to provide a discussion of the pro's and con's of different approaches. It does seem long.

shimks · 2018-03-05T20:47:18Z

I agree that it is a bit long, but I like the intent that it's going for where the testing strategy is discussed in details. Do you think that it'd be better if it was broken down into sections maybe?

jannyHou · 2018-03-05T20:57:40Z

pages/en/lb4/Testing-your-application.md

-  let app: HelloWorldApp;
-  let request: supertest.SuperTest<supertest.Test>;
+  let app: HelloWorldApplication;
+  let client: supertest.SuperTest<supertest.Test>;


I think you can simplify it by let client: Client and import Client from @loopback/testlab

See https://github.com/strongloop/loopback-next/blob/master/packages/testlab/src/client.ts#L15

jannyHou

👍 great update, LGTM! just a few nitpick.

shimks requested a review from bschrammIBM as a code owner February 23, 2018 19:06

shimks added the needs-priority label Feb 23, 2018

b-admike reviewed Feb 23, 2018

View reviewed changes

shimks changed the title ~~update Testing your application~~ [WIP] update Testing your application Feb 23, 2018

shimks force-pushed the update-testing-doc branch 3 times, most recently from a21263b to 3b71599 Compare February 23, 2018 19:28

kjdelisle reviewed Feb 23, 2018

View reviewed changes

shimks force-pushed the update-testing-doc branch from 3b71599 to abc426f Compare February 26, 2018 21:47

shimks changed the title ~~[WIP] update Testing your application~~ [DO NOT REVIEW] update Testing your application Feb 28, 2018

shimks force-pushed the update-testing-doc branch 2 times, most recently from 73ed168 to 37a8169 Compare March 2, 2018 22:22

shimks changed the title ~~[DO NOT REVIEW] update Testing your application~~ Update Testing your application Mar 2, 2018

shimks force-pushed the update-testing-doc branch from 37a8169 to df9495d Compare March 2, 2018 22:24

bschrammIBM suggested changes Mar 3, 2018

View reviewed changes

shimks force-pushed the update-testing-doc branch from df9495d to 337af4d Compare March 5, 2018 15:36

virkt25 reviewed Mar 5, 2018

View reviewed changes

shimks force-pushed the update-testing-doc branch from da164ed to cd2413c Compare March 5, 2018 17:45

virkt25 approved these changes Mar 5, 2018

View reviewed changes

jannyHou reviewed Mar 5, 2018

View reviewed changes

b-admike approved these changes Mar 5, 2018

View reviewed changes

bschrammIBM suggested changes Mar 5, 2018

View reviewed changes

jannyHou reviewed Mar 5, 2018

View reviewed changes

bschrammIBM suggested changes Mar 5, 2018

View reviewed changes

jannyHou reviewed Mar 5, 2018

View reviewed changes

jannyHou approved these changes Mar 5, 2018

View reviewed changes

shimks force-pushed the update-testing-doc branch from cd2413c to cff5c49 Compare March 5, 2018 21:16

shimks mentioned this pull request Mar 9, 2018

docs: delete calling other apis page #641

Closed

shimks force-pushed the update-testing-doc branch 2 times, most recently from e71e09c to db62445 Compare March 9, 2018 20:41

shimks added 7 commits March 9, 2018 16:00

update Testing your application

2f1ce7b

add line breaks

9859527

apply more feedback

c9785ec

add linting

469bf81

update v2 to v3

087503b

update to openapi-v3

a841944

add @bschrammIBM edits

8e6a6db

shimks force-pushed the update-testing-doc branch from db62445 to 8e6a6db Compare March 9, 2018 21:02

shimks merged commit 62245bc into gh-pages Mar 9, 2018

shimks deleted the update-testing-doc branch March 9, 2018 21:42

shimks removed the needs-priority label Mar 9, 2018

shimks mentioned this pull request Mar 15, 2018

Docs/finish migration from loopback.io loopbackio/loopback-next#1141

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Testing your application #635

Update Testing your application #635

shimks commented Feb 23, 2018 •

edited

Loading

b-admike Feb 23, 2018

kjdelisle left a comment

kjdelisle Feb 23, 2018

kjdelisle Feb 23, 2018

kjdelisle Feb 23, 2018

shimks commented Feb 28, 2018

bajtos commented Mar 1, 2018

bschrammIBM left a comment •

edited

Loading

virkt25 Mar 5, 2018

virkt25 Mar 5, 2018

jannyHou left a comment

bschrammIBM left a comment

jannyHou Mar 5, 2018

shimks Mar 5, 2018 •

edited

Loading

bschrammIBM left a comment

shimks commented Mar 5, 2018

bschrammIBM commented Mar 5, 2018

shimks commented Mar 5, 2018

jannyHou Mar 5, 2018 •

edited

Loading

jannyHou left a comment


		const details = await controller.getDetails(1);
		const details = await controller.getDetails('pen');

Update Testing your application #635

Update Testing your application #635

Conversation

shimks commented Feb 23, 2018 • edited Loading

b-admike Feb 23, 2018

Choose a reason for hiding this comment

kjdelisle left a comment

Choose a reason for hiding this comment

kjdelisle Feb 23, 2018

Choose a reason for hiding this comment

kjdelisle Feb 23, 2018

Choose a reason for hiding this comment

kjdelisle Feb 23, 2018

Choose a reason for hiding this comment

shimks commented Feb 28, 2018

bajtos commented Mar 1, 2018

bschrammIBM left a comment • edited Loading

Choose a reason for hiding this comment

virkt25 Mar 5, 2018

Choose a reason for hiding this comment

virkt25 Mar 5, 2018

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

bschrammIBM left a comment

Choose a reason for hiding this comment

jannyHou Mar 5, 2018

Choose a reason for hiding this comment

shimks Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

bschrammIBM left a comment

Choose a reason for hiding this comment

shimks commented Mar 5, 2018

bschrammIBM commented Mar 5, 2018

shimks commented Mar 5, 2018

jannyHou Mar 5, 2018 • edited Loading

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

shimks commented Feb 23, 2018 •

edited

Loading

bschrammIBM left a comment •

edited

Loading

shimks Mar 5, 2018 •

edited

Loading

jannyHou Mar 5, 2018 •

edited

Loading