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

(maint) Update clojure/test.check to match other projects #99

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Magisus
Copy link

@Magisus Magisus commented Oct 2, 2024

Jar-jar and puppetdb now use version 1.1.1 of clojure.test/check, so this library's pin to 0.9.0 causes dependency confusion in projects that depend on this and either of those. This commit updates the version used here to match the rest of the PE ecosystem.

@Magisus Magisus requested a review from a team as a code owner October 2, 2024 16:12
project.clj Outdated
@@ -8,7 +8,7 @@
:pedantic? :abort
:dependencies [[org.clojure/clojure]
[org.clojure/java.jdbc]
[org.clojure/test.check "0.9.0"]
[org.clojure/test.check "1.1.1"]
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be in the test dependencies so it's not transitively picked up by other projects?

Then this whole situation goes away.

Copy link
Author

Choose a reason for hiding this comment

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

Probably, come to think of it. A thing that may not have been known when it was added 8 years ago 🤔 Happy to move it there if we think that's fine.

Copy link
Author

Choose a reason for hiding this comment

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

It does only seem to be used in tests (wanted to make sure it wasn't used in helpers for other libraries' tests). I'll move it.

@Magisus Magisus force-pushed the update-clojure-test-check branch from 8626d66 to 3ff3706 Compare October 2, 2024 19:21
@Magisus
Copy link
Author

Magisus commented Oct 2, 2024

I added a thing to trigger tests on changes to project.clj, seems like if we're changing dependencies we should run tests.

This commit moves the `clojure/test.check` dependency to the `test` lein
profile, since it is only used in tests. This will prevent it from being
picked up transitively by other projects that depend on `jdbc-util`.
Previously, this could sometimes cause ambiguous dependency resolution,
since other projects, notably jar-jar and puppetdb, also pull in
`clojure/test.check`, recently at a different version than this library.

In addition to moving it to a test dependency, this commit also updates
the test library to the version used by other projects in the PE
ecosystem.
@Magisus Magisus force-pushed the update-clojure-test-check branch 2 times, most recently from 99890b0 to af3240f Compare October 2, 2024 20:05
@Magisus
Copy link
Author

Magisus commented Oct 2, 2024

The test failures appears to be related to this https://www.postgresql.org/message-id/tencent_BD6B866E25069560051D16A1EEA07A4F2908%40qq.com and seems legit. I think this is the offending code:

(format (str "GRANT %s TO %s"
";" create-db-statement
";REVOKE %s FROM %s")
.

ERROR in (create-db!-test) (QueryExecutorImpl.java:2676)
Uncaught exception, not in assertion.
expected: nil
  actual: org.postgresql.util.PSQLException: ERROR: CREATE DATABASE cannot be executed within a pipeline

It was missed when we upgraded from Postgres 9 to 14 here, because the tests are gated to only run on changes to src/ and test/. In this PR I added project.clj to that list, but that wouldn't have caught this either. I downgraded back to 9 and the failure went away. This feels like a real bug to me, but is unrelated to the changes in this PR.

@nwolfe
Copy link

nwolfe commented Oct 2, 2024

I would remove the paths from GHA workflows so tests are ran when CI files change, which would've caught the issue introduced at dff9519

PRs are few and far between, and tests are quick enough, just run them all for any change.

To fix the issue now, we could try pinning to the latest image that does work?

@Magisus Magisus force-pushed the update-clojure-test-check branch 3 times, most recently from ff3a487 to 7df6318 Compare October 2, 2024 20:39
Postgres 14.7 started enforcing a restriction where databases could not
be created or dropped as part of a pipeline of SQL commands. We are
doing that in our `create-db` function when the user that is designated
to be the DB owner is not part of the `db-owner` role or a superuser.
That code should be fixed, though the fact that this isn't causing a
problem in PE, which is currently using Postgres 14.13, likely means
this path is unused.

In the meantime, this commit pins CI for this repo to 14.6, where the
existing code is still allowed.
@Magisus Magisus force-pushed the update-clojure-test-check branch from 7df6318 to 525ccc2 Compare October 2, 2024 20:48
@steveax
Copy link
Contributor

steveax commented Oct 3, 2024

If it's quick (an hour or two), I say we fix the create-db! bug, otherwise, let's ticket it and fix later.

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