-
Notifications
You must be signed in to change notification settings - Fork 386
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
lb4: rework Testing your application #479
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall comments:
- Capitalize almost all words in a title except for conjunctions and prepositions ("for", "of", "with", etc.)
- Prefer use of future tense without possession for titles:{X} your {Y}
-> {X}ing {Y}
(ex. Test your foos with magic
--> Testing Foos with Magic
)
Ignore these, I didn't realize our style guide requires this.
- Remove random capitalization of certain words ("Test" was the word most commonly capitalized outside of the beginning of a sentence or title).
- Don't join "unit test" with a hyphen (
unit-test
). - Avoid starting sentences with "and".
- Prevents regressions from new features/bug fixes (code introductions and refactorings) (after deployment) | ||
- Helps new and existing developers understand different parts of the codebase (knowledge sharing) | ||
- Speeds up development over the long run (the code writes itself!) | ||
- And much more. See [benefits of software testing](http://lmgtfy.com/?q=benefits+of+software+testing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be a bullet point if it's a continuation of the thought and not the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as I find it hilarious to use LMGTFY, I don't think we should be passive-aggressive about helping people see the value of test-driven development. :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re
Capitalize almost all words in a title except for conjunctions and prepositions...
We use sentence case for headings; cf. http://loopback.io/doc/en/contrib/Doc-style-guide.html#headings
|
||
An automated test suite requires a test runner - a tool that can execute all your tests and produce a test report. We use and recommend [Mocha](https://mochajs.org). | ||
|
||
In addition to a test runner, tests need various utilities like an assertion library (we recommend [Should.js](https://shouldjs.github.io)), a library for making HTTP calls and verifying their results (we recommend [supertest](https://github.com/visionmedia/supertest)), a library for creating Test doubles (we recommend [Sinon.JS](http://sinonjs.org/)). While it's possible to install all these tools individually, we found that a good setup requires additional glue to integrate all these individual tools into something that's easy to use. Especially when writing the tests in TypeScript, due to relatively lower quality of official type definitions. We have addressed all these needs by creating our own swiss-army-knife module for testing called [@loopback/testlab](https://www.npmjs.com/package/@loopback/testlab). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break this line into multiple lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a library for creating Test doubles...
-- lower-case the word "Test".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's possible to install all these tools individually, we found that a...
"we've found that a..."
{% include note.html content="We recommend against using test mocks. With test mocks, the expectations must be defined before the tested scenario is executed, which breaks the recommended test layout 'arrange-act-assert' (or 'given-when-then') and produces code that's difficult to comprehend. | ||
" %} | ||
|
||
#### Create a stub Repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Creating Stub Repositories"
|
||
#### Create a stub Repository | ||
|
||
When writing an application accessing data in a database, it's a best practice to use [Repositories](./Repositories.html) to encapsulate all data-access/persistence-related code and let other parts of the application (typically [Controllers](./Controllers.html)) to depend on these Repositories for data access. In order to test Repository dependants (e.g. Controllers) in isolation, we need to provide a test double, usually as a test stub. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break up into multiple lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let other parts of the application (typically [Controllers](./Controllers.html)) to depend on these Repositories...
should be
let other parts of the application (typically [Controllers](./Controllers.html)) depend on these Repositories...
|
||
When writing an application accessing data in a database, it's a best practice to use [Repositories](./Repositories.html) to encapsulate all data-access/persistence-related code and let other parts of the application (typically [Controllers](./Controllers.html)) to depend on these Repositories for data access. In order to test Repository dependants (e.g. Controllers) in isolation, we need to provide a test double, usually as a test stub. | ||
|
||
In traditional object-oriented languages like Java or C#, in order to allow the unit tests to provide a custom implementation of the repository API, the controller needs to depend on an interface describing the API, and the repository implementation needs to implement this interface. The situation is easier in JavaScript and TypeScript. Thanks to the dynamic nature of the language, it’s possible to mock/stub entire classes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Break this up into multiple lines.
### Test Sequence customizations | ||
|
||
|
||
## EVERYTHING BELOW IS LEGACY AND SHOULD BE REMOVED BEFORE LANDING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert this to an HTML comment instead?
pages/en/lb4/Thinking-in-LoopBack.md
Outdated
@@ -355,6 +355,8 @@ describe('API specification', () => { | |||
}); | |||
``` | |||
|
|||
See [Validate your OpenAPI specification](./Testing-your-application.html#validate-your-openapi-specification) from Testing your application for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update link based on feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing your application
-> testing your application
pages/en/lb4/Thinking-in-LoopBack.md
Outdated
@@ -525,6 +527,8 @@ It's a powerful proposition to use the API spec not only for API declaration but | |||
|
|||
At this point, we are ready to make these tests pass by coding up your business logic. | |||
|
|||
Please refer to [Perform an auto-generated smoke test of your REST API](./Testing-your-application.html#perform-an-auto-generated-smoke-test-of-your-rest-api) from Testing your application for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing
-> testing
pages/en/lb4/Thinking-in-LoopBack.md
Outdated
@@ -982,6 +990,8 @@ The new unit test is passing now, but our integration and acceptance tests are b | |||
2. Fix the acceptance test by annotating `ProductController`'s `repository` argument with `@inject('repositories.Product')` | |||
and binding the `ProductRepository` in the main application file where we are also binding controllers. | |||
|
|||
Learn more about Controller unit testing in [Unit-test your Controllers](./Testing-your-application.html#unit-test-your-controllers) from Testing your application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update link based on feedback.
pages/en/lb4/Thinking-in-LoopBack.md
Outdated
@@ -682,6 +688,8 @@ When you scroll up in the test output, you will see more information about the 4 | |||
Unhandled error in GET /product/ink-pen: 404 Error: Controller method not found: ProductController.getDetails | |||
``` | |||
|
|||
Learn more about acceptance testing in [Test your individual REST API endpoints](./Testing-your-application.html#test-your-individual-rest-api-endpoints) from Testing your application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update link based on feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of Kevin's comments re wording, grammar, and style are fine. However, I'll copy edit the article once the technical content is in place and make any remaining changes of this nature. So if you don't have time or the inclination to do these now, I can handle them post-merge.
|
||
### With LoopBack CLI | ||
An automated test suite is important to ensure your application works as expected, prevent regressions as new features are added and give you confidence to refactor your codebase to keep it easy to further modify. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest are added, and give you confidence to continuously refactor your codebase.
here
|
||
If you have an existing application you'll need to install a few packages to make everything work well together. Minimally, you will need `@loopback/testlab` and `mocha`. | ||
If you have an existing application you'll need to install `mocha` and `@loopback/testlab` to make everything work well together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make it more clear by adding ... to make everything work well together. Inside your application root directory, run:
findStub.resolves([{id: 1, name: 'Pen'}]); | ||
``` | ||
|
||
Verify how was the stubbed method executed at the end of your unit-test (in the "assert" or "then" section): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how the stubbed method was executed
@kjdelisle @b-admike Thank you for the feedback. I'd rather leave the copy editing and style/grammar fixes to @crandmck, as he suggested. It will be more effective when he makes the edits directly, instead of having to go through your comments and apply them in all places. Is there anything else beside my style/grammar that you would like to change or improve? I pushed another commit with some new content, you may want to take another look. |
Done. @strongloop/loopback-devs PTAL at the content, please focus your review on the technical part (are my advices good/correct, the code examples easy to understand, etc.) I'll leave the formal side (style, grammar, etc.) to @crandmck to improve in a follow-up pull request. If you feel some formal problem must be fixed before this can be landed, then please make the edits yourself directly in my branch - for example using GitHub's online editor:
I was using capital letters to make special terms stand out, e.g. "Test doubles", "Controllers", "Repositories", etc. Feel free to change it to whatever makes more sense. Similarly, plain-text "Testing your application" was meant as a reference to a docs page. I changed the plain-test occurrences to a hyperlink to make my intent more clear. Having said that, I am not sure if it's a good idea to have two links everywhere - one pointing to a section of "Testing your application", another pointing to the top of that docs page. Feel free to change that. |
- integration testing (all subsections) - acceptance testing (validate api spec, smoke test api spec)
- Test your individual REST API endpoints - Test Sequence customizations
345074a
to
395ed92
Compare
1. Write just enough code to make it pass. | ||
1. Continue writing integration tests until satisfied. | ||
1. Run the acceptance test to ensure everything continues to work together as a whole. | ||
Custom sequence behavior is best tested by observing changes in behavior of affected endpoints. For example, if your sequence involves authentication step that rejects anonymous requests for certain endpoints, then you can write a test making an anonymous request to such endpoint to verify that it's correctly rejected. These tests are esssentially the same as the test verifying implementation of individual endpoints as described in the previous section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if this is the best advice, especially considering our own advice in Thinking in LoopBack:
How to create a great test suite? By thinking smaller and favoring fast, focused unit-tests over slow application-wide end-to-end tests.
On one hand, writing several end-to-end tests to verify how auth is applied to each endpoint brings quite a lot of confidence for the security of our app. On the other hand, these tests are repetitive to write and take a long time to execute.
Perhaps we should look for better ways, for example:
-
A small number of integration tests to make sure all sequence customizations are applied for HTTP requests, these tests can invoke a dummy test controller method instead of real API endpoints - we are interested in the sequence, not any specific controller method.
-
Unit test for each controller method to ensure the method is correctly decorated with the metadata used by additional sequence actions used in our custom sequence.
For example, let's say we are using @loopback/authentication
and its @authenticate
decorator and authenticate()
sequence action. To test such app, we would write:
-
four integration tests: two tests for a controller method decorated with
@authenticate
(an anonymous request, an authenticated request), two tests for a controller method not decorated with@authenticate
(again, an anonymous request, an authenticated request) -
for each controller method, one unit test verifying that the method is (or is not) decorated with
@authenticate
@kjdelisle thoughts?
If we agree to rewrite this section, then I am proposing to do so in a follow-up pull request, so that this big patch can be landed ASAP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened a p2 issue to keep track of my suggestion: loopbackio/loopback-next#648
// test/helpers/database.helpers.ts | ||
|
||
export async function givenEmptyDatabase() { | ||
await new ProductRepository().deleteAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to recommend using docker to setup a clean database?
-
from my experience api like
deleteAll()
some times also interferes testing result/pollute db if it has bug and could not clean up the old data entirely. -
you can also leave the container there instead of deleting it after test :)
I feel we benefit a lot after lb3 connectors all use container for testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are running givenEmptyDatabase
before each test. How long does it take to restart a docker container with a database? I am concerned that cleaning the database via docker restart would make the test suite extremely slow.
from my experience api like deleteAll() some times also interferes testing result/pollute db if it has bug and could not clean up the old data entirely.
I see your point. This can happen for example when there are foreign keys setup between tables and let' say a Category
record cannot be deleted until all Product
records belonging to that category are deleted too. IMO, this is a missing feature of LoopBack - we should support cascading deletes. Fortunately, this particular example has a workaround - just re-order deleteAll
statements so that products are deleted before categories.
I think we shouldn't be recommending our users suboptimal solutions just to avoid shortcomings in LoopBack and/or the way how the users designed their app & data schema. What I am proposing instead: let's use this as an opportunity to identify missing gaps and either explain users how to work around them, or work together to fix these issues.
@jannyHou What do you think, should I add a short paragraph explaining how to clear database when there are relationships between tables enforced by the DB engine, like I shown in my first paragraph?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos yeah the time is a problem for starting a container...
I think we can add more after figure out a plan for persistence story, now your content is good enough, going into too much details might be off topic.
Rand will take care of fixing my grammar and style.
Update "Testing your application" to match the content in "Thinking in LoopBack", remove "Advanced testing".
Update "Testing your application" to match the content in "Thinking in LoopBack", remove "Advanced testing". Add Extending LoopBack 4 Fix Jekyll errors, etc Update Extending LoopBack
Update "Testing your application" to match the content in "Thinking in LoopBack", remove "Advanced testing". Add Extending LoopBack 4 Fix Jekyll errors, etc Update Extending LoopBack
Update "Testing your application" to match the content in "Thinking in LoopBack", remove "Advanced testing". lb4: rework Testing your application (#479) Update "Testing your application" to match the content in "Thinking in LoopBack", remove "Advanced testing". Add Extending LoopBack 4 Fix Jekyll errors, etc Update Extending LoopBack
"Testing your application" and "Testing-Your-application-advanced" are behind "Thinking in LoopBack", they are also partially duplicating the content in that page. This pull request contains a rework/rewrite of these two "Testing" pages. See loopbackio/loopback-next#553 for more details.
Connect to loopbackio/loopback-next#553
In contrary to Acceptance Criteria described in the lb-next issue, I decided to keep all code examples in Thinking in LoopBack without changes. IMO, these examples are important to keep "Thinking in LoopBack" as a self-contained guide that can be read on its own, without having to jump to different pages and back.
This is work-in-progress and should not be landed yet.
Tasks