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

test: add acceptance-level tests for repository.execute() #6046

Merged
merged 3 commits into from
Aug 11, 2020

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Aug 3, 2020

See #3342

WAITING FOR loopbackio/loopback-datasource-juggler#1857

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 added the Repository Issues related to @loopback/repository package label Aug 3, 2020
@bajtos bajtos requested a review from raymondfeng August 3, 2020 15:16
@bajtos bajtos self-assigned this Aug 3, 2020
@raymondfeng
Copy link
Contributor

Please investigate CI failures.

@bajtos
Copy link
Member Author

bajtos commented Aug 4, 2020

MongoDB tests are waiting for loopbackio/loopback-datasource-juggler#1857, sorry for not mentioning this in the PR description.

I was not expecting PostgreSQL failure, I'll investigate.

1) PostgreSQL repository.execute()
     executes a parameterized native SQL query:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; 
     if returning a Promise, ensure it resolves. 
     (/home/travis/build/strongloop/loopback-next/acceptance/repository-postgresql/dist/__tests__/postgresql-repository-execute.acceptance.js)

@bajtos bajtos force-pushed the test/repository-execute branch from 77830dd to b062088 Compare August 4, 2020 08:21
@bajtos
Copy link
Member Author

bajtos commented Aug 4, 2020

I am not able to identify why is the new PostgreSQL test failing. It passes when I execute it on its own, but fails on connection timeout when executed after the shared repository-test suite. I suspect there is a bug in the way how we deal with datasource initialization/cleanup and/or in PostgreSQL's pooling implementation. There were several issues reported in the past months where the connection process never finished (i.e. what we are observing too), they were fixed in multiple 8.x versions. Maybe there is another bug lurking somewhere.

I am proposing to skip the failing test for now and investigate the problem more either as part of #3342 or in a follow-up issue.

@bajtos bajtos force-pushed the test/repository-execute branch from b062088 to e461787 Compare August 4, 2020 16:29
bajtos added 3 commits August 10, 2020 10:24
Before this change, when running tests locally, the environment was
configured to use the database names `testdb` and `testdb_new`. However,
the setup SQL script is creating `repository_tests` and
`repository_tests_new` databases instead. As a result, the test
accessing `*_new` database was failing.

This commit fixes `setup.sh` script to export `POSTGRESQL_DATABASE`
environment variable to tell the test suite to use the desired database.

Signed-off-by: Miroslav Bajtoš <[email protected]>
@bajtos bajtos force-pushed the test/repository-execute branch from e461787 to 796c027 Compare August 10, 2020 09:22
@bajtos
Copy link
Member Author

bajtos commented Aug 10, 2020

The PR is ready for final review & landing, PTAL. @raymondfeng @hacksparrow @jannyHou @agnes512

describe('PostgreSQL repository.execute()', () => {
// FIXME(bajtos) This test passes when executed in isolation, but fails
// on connection timeout when executed after repository-test suite.
it.skip('executes a parameterized native SQL query', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm interesting. It is indicative of some sort of bug somewhere (maybe a shared global state somehere?). I think we should resolve this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I agree this needs to be fixed. See #6046 (comment)

I am proposing to skip the failing test for now and investigate the problem more either as part of #3342 or in a follow-up issue.

My primary motivation in this pull request is to ensure MongoDB commands can be executed. I don't want to keep this pull request open for too long, to avoid merge conflicts.

I am going to investigate the failing test as part of #3342, I added a sub-task to the TODO list.

@hacksparrow
Copy link
Contributor

What about support for other databases?

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

👍

@bajtos
Copy link
Member Author

bajtos commented Aug 11, 2020

What about support for other databases?

That's a valid concern. We don't have acceptance-level tests for other databases yet, adding such test suite is out of scope of this work 🤷‍♂️

@bajtos bajtos merged commit 8cb2031 into master Aug 11, 2020
@bajtos bajtos deleted the test/repository-execute branch August 11, 2020 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Repository Issues related to @loopback/repository package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants