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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a shared Repository test suite and run it against different connectors #3079

Merged
merged 1 commit into from Jun 18, 2019

Conversation

Projects
None yet
5 participants
@bajtos
Copy link
Member

commented Jun 10, 2019

This pull request creates infrastructure for writing tests for different Repositiory interfaces and executing them against different Repository implementations (e.g. DefaultCrudRepository and CrudRepositoryImpl) and different databases (e.g. MongoDB and MySQL).

This work is based on my experience building similar test suites in the past

The shared test suite has only two very basic test cases to verify the testing infrastructure. My intention is to move relevant tests from packages/repository/src/__tests__ to the new test suite in follow-up pull requests.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • 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 was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@bajtos bajtos requested a review from raymondfeng Jun 10, 2019

@bajtos bajtos self-assigned this Jun 10, 2019

@bajtos bajtos referenced this pull request Jun 11, 2019

Open

Juggler Refactor - Epic #889

@bajtos bajtos force-pushed the feat/repository-tests branch 2 times, most recently from 76b26ea to 5870b59 Jun 11, 2019

script:
- npm run postinstall -- --scope "@loopback/test-repository-mongodb" --include-filtered-dependencies --force-local
- npm run build -- --scope "@loopback/test-repository-mongodb" --include-filtered-dependencies
- cd acceptance/repository-mongodb && npm run mocha

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 11, 2019

Author Member

Note that I am intentionally leaving acceptance tests OUT OF npm test and configuring a new Travis CI job to run them in parallel. My intention is to add new scripts allowing developers to run these tests locally, but only when they decide to do so.

This comment has been minimized.

Copy link
@b-admike

b-admike Jun 17, 2019

Member

That's good, but these tasks still run and are required for passing builds right?

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 17, 2019

Author Member

Yes, these tasks are executed and are required to consider a CI build as success.

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

@bajtos Great idea! It's nice that we can parameterize resource dependencies for tests.

@bajtos bajtos referenced this pull request Jun 13, 2019

Merged

feat(testlab): add `skipIf` helper #3138

3 of 3 tasks complete

@bajtos bajtos force-pushed the feat/repository-tests branch 4 times, most recently from 26abebc to 02bba0a Jun 13, 2019

@bajtos bajtos marked this pull request as ready for review Jun 13, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

The pull request is ready for review 馃挭

@strongloop/loopback-maintainers PTAL

@bajtos bajtos requested a review from strongloop/loopback-maintainers Jun 13, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

BTW if you are concerned about CI times, the new tasks finish pretty fast (and run in parallel with existing tasks).

Screen Shot 2019-06-13 at 14 48 56

@bajtos bajtos changed the title Create a shared `@loopback/repository` test suite and run it against different connectors Create a shared Repository test suite and run it against different connectors Jun 13, 2019

@bajtos bajtos force-pushed the feat/repository-tests branch from 02bba0a to 7f69468 Jun 17, 2019

@bajtos bajtos requested a review from strongloop/loopback-maintainers Jun 17, 2019

@bajtos bajtos force-pushed the feat/repository-tests branch from 7f69468 to f4b3533 Jun 17, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

The pull request is ready for final review.

@raymondfeng @strongloop/loopback-maintainers PTAL

@b-admike
Copy link
Member

left a comment

Wow great addition 馃憦. The readme for the base @loopback/repository-tests repo has good usage instructions, but maybe it'd be good to describe or give an overview on how it works under the hood (just a thought).

Show resolved Hide resolved acceptance/repository-mongodb/setup.sh
Show resolved Hide resolved packages/repository-tests/src/helpers.repository-tests.ts Outdated
script:
- npm run postinstall -- --scope "@loopback/test-repository-mongodb" --include-filtered-dependencies --force-local
- npm run build -- --scope "@loopback/test-repository-mongodb" --include-filtered-dependencies
- cd acceptance/repository-mongodb && npm run mocha

This comment has been minimized.

Copy link
@b-admike

b-admike Jun 17, 2019

Member

That's good, but these tasks still run and are required for passing builds right?

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jun 17, 2019

@b-admike good feedback, I have addressed your comments in 6b874b9. LGTY now?

@b-admike
Copy link
Member

left a comment

Thanks for adding more docs :).

@bajtos bajtos force-pushed the feat/repository-tests branch from 6b874b9 to f1de448 Jun 17, 2019

@jannyHou
Copy link
Contributor

left a comment

LGTM with some nitpicks 馃憦

  • Please double check the clean commands to make sure the name mapping is correct.
  • You may need to add the path for the acceptance folder in the root level package.json's mocha command so that appveyor also runs the tests.
},
"scripts": {
"build": "lb-tsc",
"clean": "lb-clean loopback-test-mongodb*.tgz dist tsconfig.build.tsbuildinfo package",

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jun 17, 2019

Contributor

wouldn't the name be loopback-test-repository-mongodb*.tgz instead of loopback-test-mongodb*.tgz?

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 18, 2019

Author Member

Good catch! Will fix.

* are used by the test suite to tweak assertions and skip tests
* for scenarios not supported by some connectors.
*/
export interface CrudConnectorFeatures {

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jun 17, 2019

Contributor

Just curious :) you use interface instead of type here, is it for the convenience of using declaration merging?

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 18, 2019

Author Member

When describing object properties, interface is the keyword to use. It allows other interfaces and classes to inherit from CrudConnectorFeatures. The keyword type does not allow inheritance, it defines a kind of a pointer/reference.

@@ -0,0 +1,21 @@
// Copyright IBM Corp. 2019. All Rights Reserved.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jun 17, 2019

Contributor

(Out of the scope of this PR) Would it make more sense if move the memory acceptance test to the acceptance dir for the sake of consistency?

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 18, 2019

Author Member

Would it make more sense if move the memory acceptance test to the acceptance dir for the sake of consistency?

No.

I am intentionally keeping memory-based tests inside packages/repository-tests to ensure we execute the shared test suite as part of regular npm test and that the coverage data include lines covered by the shared test suite.

Here is the typical workflow:

  • When making changes in loopback-next, we run the shared test suite using the memory connector, because it's relatively fast.
  • We don't run the tests again databases, because it's expensive and the probability of breaking functionality in a database-specific way is very very low.
  • However, we still want to verify that our code works with real databases, that's why we have dedicated Travis CI jobs running these tests for pull requests.
  • In the rare case that we broke some of the shared tests with one or more databases, we can run the tests in acceptance/repository-{database} manually, as described in READMEs.

This comment has been minimized.

Copy link
@jannyHou

jannyHou Jun 19, 2019

Contributor

馃憤 I see fair enough. And I like running those acceptance tests separately.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

@jannyHou

You may need to add the path for the acceptance folder in the root level package.json's mocha command so that appveyor also runs the tests.

See #3079 (comment):

Note that I am intentionally leaving acceptance tests OUT OF npm test and configuring a new Travis CI job to run them in parallel. My intention is to add new scripts allowing developers to run these tests locally, but only when they decide to do so.

It's my intention to NOT RUN the new connector-based tests on AppVeyor.

feat: shared Repository test suite
Creates infrastructure for writing tests for different `Repositiory`
interfaces and executing them against different Repository
implementations (e.g. `DefaultCrudRepository` and `CrudRepositoryImpl`)
and different databases (e.g. `MongoDB` and `MySQL`).

The initial version of the shared test suite has only two very basic
test cases to verify the testing infrastructure. My intention is to move
relevant tests from `packages/repository/src/__tests__` to the new test
suite later.

@bajtos bajtos force-pushed the feat/repository-tests branch from f1de448 to e2a04c7 Jun 18, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

@jannyHou thank you for thoughtful comments. WDYT, should I capture some of my answers in README files or elsewhere in our docs? Please advise where & how.

@bajtos bajtos merged commit e9dca4c into master Jun 18, 2019

4 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.07%) to 91.745%
Details

@delete-merged-branch delete-merged-branch bot deleted the feat/repository-tests branch Jun 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.