Skip to content

Initial foray at integration embedded PostgreSQL.#40

Merged
bobmcwhirter merged 11 commits intoguacsec:mainfrom
bobmcwhirter:embedded-pgsql-for-tests
Mar 11, 2024
Merged

Initial foray at integration embedded PostgreSQL.#40
bobmcwhirter merged 11 commits intoguacsec:mainfrom
bobmcwhirter:embedded-pgsql-for-tests

Conversation

@bobmcwhirter
Copy link
Contributor

Initially only applies for #[cfg(test)] types of targets, to convert "integration tests" into "unit tests".

The special system constructor returns a handle to the temporary database process, which must be held onto through the duration of a given test, as having it Drop'd causes the DB to shutdown.

Note: this includes assigning it to _ or _db, hence using an actually named (but other unused) variable.

@bobmcwhirter bobmcwhirter requested review from ctron, jcrossley3 and mrizzi and removed request for ctron March 8, 2024 19:32
@helio-frota helio-frota added the ci Github Actions related changes label Mar 10, 2024
@ctron
Copy link
Contributor

ctron commented Mar 11, 2024

I absolutely like that. If running tests, can they still run in parallel?

@bobmcwhirter
Copy link
Contributor Author

Currently not on CI but I'm sure we can sort that out

@bobmcwhirter bobmcwhirter marked this pull request as draft March 11, 2024 13:26
Bob McWhirter added 9 commits March 11, 2024 09:52
Initially only applies for #[cfg(test)] types of targets, to
convert "integration tests" into "unit tests".

The special system constructor returns a handle to the temporary
database process, which *must be held onto* through the duration
of a given test, as having it Drop'd causes the DB to shutdown.

Note: this includes assigning it to _ or _db, hence using an
actually named (but other unused) variable.
@bobmcwhirter bobmcwhirter force-pushed the embedded-pgsql-for-tests branch from e9c45e3 to d165483 Compare March 11, 2024 13:53
@bobmcwhirter
Copy link
Contributor Author

I think ultimately we need to upstream a PR to the postgresql-embedded crate for 2 reasons:

  1. It supports Clone but also has Drop implementation which... shuts everything down. Wrapping in an Arc on our end helps that case.
  2. Multithreaded-testing results in a race on the install/init the base pgsql. Some flavor of lockfile should possibly be involved.

@bobmcwhirter bobmcwhirter marked this pull request as ready for review March 11, 2024 13:54
@bobmcwhirter
Copy link
Contributor Author

(after a first run locally, when pgsql is installed/init'd, you can run tests concurrently no problem)

Copy link
Contributor

@mrizzi mrizzi left a comment

Choose a reason for hiding this comment

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

Just a comment based on local testing.


- name: Test
run: cargo test
run: cargo test -- --test-threads=1 --nocapture
Copy link
Contributor

@mrizzi mrizzi Mar 11, 2024

Choose a reason for hiding this comment

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

Testing this locally for me it worked successfully with just cargo test. I'm wondering if, after your latest changes, --test-threads=1 is still required or it can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lemme try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect it's only a problem on a perfectly-clean system that has never run the tests before (such as a CI worker).

Copy link
Contributor

@mrizzi mrizzi left a comment

Choose a reason for hiding this comment

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

@bobmcwhirter thanks a lot 👍

@bobmcwhirter bobmcwhirter merged commit dadc483 into guacsec:main Mar 11, 2024
@helio-frota helio-frota removed the ci Github Actions related changes label Feb 11, 2025
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.

4 participants