Skip to content
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

feat(testlab): add createRestAppClient(), simplify usage in tests #1734

Merged
merged 1 commit into from
Sep 21, 2018

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Sep 20, 2018

While reviewing loopbackio/loopback4-example-shopping#12, I noticed the test code is depending on low-level types from supertest that are difficult to discover (in my experience). I decided to fix this and introduce a type alias, only to find that we already have Client alias defined! As I was replacing supertest type with Client alias, I found few places calling supertest directly instead of using our createClient* helper APIs and decided to improve that too. Here is the outcome:

  • Clean up all test code (including examples in the documentation) to use Client instead of supertest.SuperTest<supertest.Test>.
  • Remove createClientForRestServer because it was not used anywhere and had the issue of leaking a listening server (not stopping the server after the test is done).
  • Add a new helper createRestAppClient instead, rework acceptance tests (including the templates) to use this new helper.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site testlab's README was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

Copy link
Contributor

@virkt25 virkt25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this simpler approach 💯 Excellent PR.

It's marked as a feature ... I wonder if it should also be marked as a breaking change in the commit message? ... that said the effect is the same for feat and breaking change while on 0.x.y versioning.

@bajtos
Copy link
Member Author

bajtos commented Sep 21, 2018

It's marked as a feature ... I wonder if it should also be marked as a breaking change in the commit message? ... that said the effect is the same for feat and breaking change while on 0.x.y versioning.

Unfortunately, when lerna publish encounters a breaking change, it bump the version to 1.0.0 :(

See Build: lerna publish upgrades from 0.x.y to 1.0.0 for breaking changes #1068

@bajtos bajtos force-pushed the feat/supertest-client-cleanup branch from d46d91a to 3d2463d Compare September 21, 2018 06:21
Clean up all test code (including examples in the documentation)
to use `Client` instead of `supertest.SuperTest<supertest.Test>`.

Remove `createClientForRestServer` because it was not used anywhere
and had the issue of not stopping the server after the test is done.

Add a new helper `createRestAppClient` instead, rework acceptance tests
(including the templates) to use this new helper.
@bajtos bajtos force-pushed the feat/supertest-client-cleanup branch from 3d2463d to 58d3514 Compare September 21, 2018 06:37
@bajtos bajtos merged commit d75be77 into master Sep 21, 2018
@bajtos bajtos deleted the feat/supertest-client-cleanup branch September 21, 2018 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants