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

Implement running db tests locally #4431

Merged

Conversation

schuellerf
Copy link
Contributor

This PR implements make db-tests and extends make clean

The tests which are run locally until now used the fsjobqueue and the postgres DB tests were only run in github.

For now this just implements running the tests. More improvements to the tests them selves will be handled in subsequent PRs.

Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

Personally I think a compose file is a bit overkill. I don't see why we have to run two containers just to run the test, plus it's another container file to maintain for which we need to keep the go version up to date.

In the end running a single container is just a one liner. And I think it's reasonable to expect tern to be installed on the host? After all it's kind of a development dependency here.

@schuellerf
Copy link
Contributor Author

The idea is, that the setup is as similar to the github actions as possible, so installing a specific postgres version and a specific go version is not a one liner containerfile, I guess.
I'll try to shrink it, although I'm not sure if this easier to keep in sync with the github action.
For the question tern to be on the host, I thought it's more reliable and to avoid possible version hiccups this way.

@schuellerf schuellerf force-pushed the implement-running-db-tests-locally branch 2 times, most recently from 1b0ec46 to 56b8cd8 Compare October 24, 2024 09:55
@croissanne
Copy link
Member

The idea is, that the setup is as similar to the github actions as possible, so installing a specific postgres version and a specific go version is not a one liner containerfile, I guess. I'll try to shrink it, although I'm not sure if this easier to keep in sync with the github action. For the question tern to be on the host, I thought it's more reliable and to avoid possible version hiccups this way.

Personally, I think the only way this will be actively maintained is if the github workflow actually calls the make target that you're adding here.

I think the make target should just spin up a postgres container in case it's not running (the exact version doesn't matter imo, as long as it's maintained, we don't depend on a specific version of postgres) and do the test. The workflow should then just call the make target. Similar to what we do for make unit-tests.

Copy link
Member

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

LGTM overall, ig we should edit the comment though?

Makefile Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Show resolved Hide resolved
enables the option to run the DB tests locally
that are executed in the github actions
It does not run the tests because of to the "-c" flag
Not needed for the test but just generates a useless warning
@schuellerf schuellerf force-pushed the implement-running-db-tests-locally branch from a746947 to e4fb7ee Compare November 6, 2024 09:37
@schuellerf schuellerf enabled auto-merge (rebase) November 6, 2024 10:25
@schuellerf schuellerf merged commit f291f41 into osbuild:main Nov 6, 2024
47 of 50 checks passed
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.

2 participants