-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Allow to skip PostgreSQL backend dependent tests at runtime #51910
Conversation
Can be used by tests to declare their need for a qgis test db and allows users to skip them by defining a QGIS_PGTEST_DB_SKIP environment variable
CI is failing due to a bug in the workflow itself, see https://lists.osgeo.org/pipermail/qgis-developer/2023-February/065542.html |
I've filed #51917 as a spinoff of this one |
.github/workflows/run-tests.yml
Outdated
@@ -408,7 +408,9 @@ jobs: | |||
path: qgis_test_report | |||
|
|||
clang-tidy: | |||
if: github.event_name == 'pull_request' | |||
#if: github.event_name == 'pull_request' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you back out this change please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can, but it was suggested by the "clang-tidy" test author to do it beause as reported in #51917 the test is broken and would then make CI red and this PR unmergeable. See https://lists.osgeo.org/pipermail/qgis-developer/2023-February/065543.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\cc @troopa81
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how it's configured, it might still be possible to merge PRs with failing tests. Given the amount of issues with the test suite, I suspect it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a spinoff of this PR as #51935 to just contain this commit. I would wait for that to be merged before updating this PR as updating this PR would have no other effect than triggering a new CI run that's known to fail ( as you can see already in the history of this PR: https://github.com/qgis/QGIS/actions/runs/4203324613/jobs/7294511888 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how it's configured, it might still be possible to merge PRs with failing tests. Given the amount of issues with the test suite, I suspect it is.
Possibly, but I've heard more than once that developers don't even consider reviewing PRs until they have a green CI icon near them, and indeed this PR only got a review AFTER I pushed the commit disabling the broken test, which I did AFTER asking for help on the mailing list and receiving the suggestion to do the disabling here in order to "get the PR merged".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the observation you made about wanting to keep the clang-tidy test I've removed that commit from this PR. Can I get your approval on it now ? The CI run will fail, and I'm not even sure why github is running it again (maybe because master branch changed from the last time the code was checked)
24d8678
to
36bfa7b
Compare
Allows users to define a
QGIS_PGTEST_DB_SKIP
environment variable to skip tests requiring the test postgresql database to be run.This is a spin-off from #51891 in response to @nyalldawson request expressed in #51891 (comment)