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

feat(test-repository-postgresql): run repository tests for postgresql #3853

Merged
merged 1 commit into from Oct 24, 2019

Conversation

@nabdelgadir
Copy link
Contributor

nabdelgadir commented Oct 1, 2019

Resolves #3436.

Added acceptance/repository-postgresql package in order for repository-tests to be run for PostgreSQL.

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 馃憟

@nabdelgadir nabdelgadir self-assigned this Oct 1, 2019
@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch 6 times, most recently from 7e368a3 to 2f033b5 Oct 1, 2019
Copy link
Member

bajtos left a comment

Looks pretty good, assuming that tests are passing :)

@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch 8 times, most recently from e54155b to 19dfd28 Oct 3, 2019
@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch 8 times, most recently from 2e313e5 to 479c57d Oct 8, 2019
@nabdelgadir

This comment has been minimized.

Copy link
Contributor Author

nabdelgadir commented Oct 9, 2019

Re the coverage drop: the transactions test suite isn't run by the memory database, but it's run by acceptance/mysql and acceptance/postgresql.

@nabdelgadir nabdelgadir marked this pull request as ready for review Oct 9, 2019
@nabdelgadir nabdelgadir requested a review from raymondfeng as a code owner Oct 9, 2019
@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch from ce4337c to ec2028b Oct 9, 2019
@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch 2 times, most recently from 66b6bc5 to 90ffb19 Oct 9, 2019
.travis.yml Outdated Show resolved Hide resolved
Copy link
Member

bajtos left a comment

Here are steps I did to verify these changes:

git checkout postgresql-tests
npm i
npm run build
cd acceptance/repository-postgresql
source setup.sh
npm t

One of the tests failed:

  1) PostgreSQL + DefaultTransactionalRepository
       CRUD Repository operations
         transactions
           create-retrieve with transactions
             should not use transaction with another repository:
     Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/Users/bajtos/src/loopback/next/acceptance/repository-postgresql/dist/__tests__/postgresql-default-repository.acceptance.js)
      at listOnTimeout (internal/timers.js:531:17)
      at processTimers (internal/timers.js:475:7)

Could you PTAL?

acceptance/repository-postgresql/README.md Outdated Show resolved Hide resolved
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Oct 10, 2019

BTW I find it weird that the test is failing with a timeout, I would expect it to throw an error saying that the configured database does not exist. This is probably a bug in our PostgreSQL connector, therefore out of scope of this pull request, but possibly worth fixing or at least opening a GH issue for.

@emonddr

This comment has been minimized.

Copy link
Contributor

emonddr commented Oct 17, 2019

BTW I find it weird that the test is failing with a timeout, I would expect it to throw an error saying that the configured database does not exist. This is probably a bug in our PostgreSQL connector, therefore out of scope of this pull request, but possibly worth fixing or at least opening a GH issue for.

@nabdelgadir ^ please respond to this comment

@emonddr

This comment has been minimized.

Copy link
Contributor

emonddr commented Oct 17, 2019

@nabdelgadir

Regarding

Re the coverage drop: the transactions test suite isn't run by the memory database, but it's run by acceptance/mysql and acceptance/postgresql.

Do you have a proposed fix (new set of tests) to bring the coverage back up to an appropriate level?

Right now, the CI is complaining

image

@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch from 90ffb19 to c9e050e Oct 18, 2019
@nabdelgadir

This comment has been minimized.

Copy link
Contributor Author

nabdelgadir commented Oct 18, 2019

BTW I find it weird that the test is failing with a timeout, I would expect it to throw an error saying that the configured database does not exist. This is probably a bug in our PostgreSQL connector, therefore out of scope of this pull request, but possibly worth fixing or at least opening a GH issue for.

@bajtos I tried creating an app to reproduce this error, and it actually does throw a descriptive error (e.g. error: database "testdb" does not exist) and it throws it if I run that test individually. So the connector is throwing the error properly. It might be hard to resolve this issue if it needs this specific setup in order to be tested.

@nabdelgadir

This comment has been minimized.

Copy link
Contributor Author

nabdelgadir commented Oct 18, 2019

Regarding

Re the coverage drop: the transactions test suite isn't run by the memory database, but it's run by acceptance/mysql and acceptance/postgresql.

Do you have a proposed fix (new set of tests) to bring the coverage back up to an appropriate level?

Right now, the CI is complaining

image

@emonddr Not really sure how to bring it back up because as I said the coverage only accounts for tests run by the memory database and the memory database doesn't support transactions. So the coverage for the file it's complaining about can't be brought back up. But do you have something in mind?

@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch 2 times, most recently from 42c548d to 4f0ef07 Oct 18, 2019
@@ -30,7 +30,7 @@ matrix:
env: TASK=code-lint
# Running Code Linter -- Requires @loopback/build so it's bootstrapped
script:
- lerna bootstrap --scope @loopback/build --include-filtered-dependencies
- lerna bootstrap --scope @loopback/build --include-dependencies

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Oct 18, 2019

Author Contributor

Screen Shot 2019-10-18 at 12 21 21 AM

Replacing because --include-filtered-dependencies has been renamed.

@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch from 4f0ef07 to ab40134 Oct 18, 2019
@emonddr

This comment has been minimized.

Copy link
Contributor

emonddr commented Oct 23, 2019

@slnode test please

@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch from ab40134 to eced00b Oct 23, 2019
@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch from eced00b to 48d6e0b Oct 24, 2019
@emonddr

This comment has been minimized.

Copy link
Contributor

emonddr commented Oct 24, 2019

@nabdelgadir , did you run the tests locally and all tests passed? Referring to @bajtos comment in #3853 (review) .

@nabdelgadir

This comment has been minimized.

Copy link
Contributor Author

nabdelgadir commented Oct 24, 2019

@nabdelgadir , did you run the tests locally and all tests passed? Referring to @bajtos comment in #3853 (review) .

Yes, they work locally.

@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch from 48d6e0b to b5a31e4 Oct 24, 2019
@nabdelgadir nabdelgadir merged commit 8d029c4 into master Oct 24, 2019
4 checks passed
4 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.3%) to 92.098%
Details
@nabdelgadir nabdelgadir deleted the postgresql-tests branch Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.