Skip to content

Conversation

@nabdelgadir
Copy link
Contributor

@nabdelgadir 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 Compare October 3, 2019 00:13
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

Looks pretty good, assuming that tests are passing :)

@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch 8 times, most recently from e54155b to 19dfd28 Compare October 8, 2019 13:50
@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch 8 times, most recently from 2e313e5 to 479c57d Compare October 9, 2019 15:17
@nabdelgadir
Copy link
Contributor Author

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 October 9, 2019 16:58
@nabdelgadir nabdelgadir force-pushed the postgresql-tests branch 2 times, most recently from 66b6bc5 to 90ffb19 Compare October 9, 2019 21:56
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

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?

@bajtos
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
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
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 Compare October 18, 2019 04:20
# 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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

@emonddr
Copy link
Contributor

emonddr commented Oct 23, 2019

@slnode test please

@emonddr
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
Copy link
Contributor Author

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

Yes, they work locally.

@nabdelgadir nabdelgadir merged commit 8d029c4 into master Oct 24, 2019
@nabdelgadir nabdelgadir deleted the postgresql-tests branch October 24, 2019 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Run repository tests for PostgreSQL

6 participants