pgjdbc/pgjdbc GitHub issues and pull requests (mirror)
help / color / mirror / Atom feed[pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
16+ messages / 3 participants
[nested] [flat]
* [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-14 18:35 "sehrope (@sehrope)" <[email protected]>
0 siblings, 0 replies; 16+ messages in thread
From: sehrope (@sehrope) @ 2026-02-14 18:35 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Three commits here which:
* Add a new `@RequireServerVersion` annotation for tests that allows declaring the applicable server versions.
* Replaces the `@DisabledIfServerVersionBelow` and `@DisabledIfServerVersionGreater` usage with `@RequireServerVersion`
* Removes the unused annotations
Main driver for this was trying to understand which test is running when. The double negative for the annotation vs the check makes it confusing (e.g., it runs when it's not disabled and that is driven by the version being below...).
It's also ambiguous if something like "*Greater than 10*" really means greater than or equal 10.0.
The new approach positive affirms the situations where we want the thing to run with annotation args for:
* `lt` - Less than
* `lte` - Less than or equal
* `gt` - Greater than
* `gte` - Greater than or equal
I'd expect that `lt` and `gte` are going to be the most useful. The "less than" makes sense if you want all versions older than X (where presumably it breaks). And "greater than or equal" makes sense when X is the first version that supports a given feature that is being tested. Ex:
```diff
@@ -297,7 +297,7 @@ class LogicalReplicationStatusTest {
* Test fail on PG version 9.4.5 because postgresql have bug.
*/
@Test
- @DisabledIfServerVersionBelow("9.4.8")
+ @RequireServerVersion(gte = "9.4.8")
void applyLocationDoNotDependOnFlushLocation() throws Exception {
PGConnection pgConnection = (PGConnection) replicationConnection;
```
Note that the old `@DisabledIfServerVersionGreater("19")` name implied a `>` check, but its implementation actually used `>=`, disabling the test for version 19 and above. The migration to `@RequireServerVersion(lt = "19")` preserves that existing runtime behavior while making the boundary semantics explicit.
One minor improvement to the old approach was caching the server response in the junit context. That shaves a connection open / close per annotation call. I doubt it's meaningfully different (24 uses of the annotation times 10ms per connection rounds to nothing) but nice to have an example of that in the test code. It may be useful somewhere else too.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-14 18:48 ` "vlsi (@vlsi)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: vlsi (@vlsi) @ 2026-02-14 18:48 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
I would suggest going with junit-like naming: `@DisabledForServerVersionRange`.
See
* https://docs.junit.org/5.13.0/api/org.junit.jupiter.api/org/junit/jupiter/api/condition/DisabledForJ...
* https://docs.junit.org/5.13.0/api/org.junit.jupiter.api/org/junit/jupiter/api/condition/package-summ...
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-14 21:40 ` "sehrope (@sehrope)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: sehrope (@sehrope) @ 2026-02-14 21:40 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Renamed to `@EnabledForServerVersionRange`. I kept it positive (i.e. "EnabledFor" v.s. "DisabledFor") as the whole point is that it's easier to reason about what it's doing when you don't have to double negate it.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-14 22:04 ` "vlsi (@vlsi)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: vlsi (@vlsi) @ 2026-02-14 22:04 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> EnabledFor v.s. DisabledFor
It might be reasonable to have both.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-15 11:13 ` "davecramer (@davecramer)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: davecramer (@davecramer) @ 2026-02-15 11:13 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
> > EnabledFor v.s. DisabledFor
>
> It might be reasonable to have both.
I agree, It's plain to say disabled for.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-16 15:33 ` "sehrope (@sehrope)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: sehrope (@sehrope) @ 2026-02-16 15:33 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Do you mean add `@DisabledForServerVersionRange` (and it's condition handler) with nothing using them as an option for the future? Or replace the `@EnabledForServerVersionRange` with `@DisabledForServerVersionRange`?
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-16 16:18 ` "vlsi (@vlsi)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: vlsi (@vlsi) @ 2026-02-16 16:18 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
I mean we could add `DisabledForServerVersionRange` either now (even if it not used) or later when it would be helpful.
Having both at the same time might be helpful for different cases.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 10:55 ` "davecramer (@davecramer)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: davecramer (@davecramer) @ 2026-02-23 10:55 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
@sehrope are we waiting for something? Might as well merge this
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 11:49 ` "vlsi (@vlsi)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: vlsi (@vlsi) @ 2026-02-23 11:49 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on testkit/src/main/java/org/postgresql/test/annotations/EnabledForServerVersionRange.java)
Should the javadoc be reworded to avoid double negation as well? (disables .. does not..)
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 11:50 ` "vlsi (@vlsi)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: vlsi (@vlsi) @ 2026-02-23 11:50 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on testkit/src/main/java/org/postgresql/test/annotations/EnabledForServerVersionRange.java:33)
There's no check for contradictory specs like `@EnabledForServerVersionRange(lt = "9.4", gt = "10.0")` or specifying both lt and lte simultaneously. Worth at least validating that lt/lte aren't both set, and same for gt/gte.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 11:50 ` "vlsi (@vlsi)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: vlsi (@vlsi) @ 2026-02-23 11:50 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on testkit/src/main/java/org/postgresql/test/impl/EnabledForServerVersionRangeCondition.java)
If someone writes `@EnabledForServerVersionRange` with no parameters at all, the annotation is found but all four bounds are empty — so it silently does nothing and the test is enabled. It would be better to fail fast with an error if none of lt/lte/gte/gt are set.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 12:02 ` "vlsi (@vlsi)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: vlsi (@vlsi) @ 2026-02-23 12:02 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on testkit/src/main/java/org/postgresql/test/annotations/EnabledForServerVersionRange.java:33)
> Worth at least validating that lt/lte aren't both set
An alternative option could be: `String min, String max, boolean minInclusive, boolean maxInclusive`.
Almost all the usages of the new annotation are `gte`, so we can go with `minInclusive = true`, `maxInclusive = false` by default. It would match the typical half-open ranges like in `List.subList(int fromIndex, int toIndex)`, and it would make `lte + lt` unrepresentable.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 12:05 ` "vlsi (@vlsi)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: vlsi (@vlsi) @ 2026-02-23 12:05 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
For reference, JUnit has both `@DisabledForJreRange` and `@EnabledForJreRange` because their `min` and `max` are always `inclusive`, so they need two annotations to distinguish between `less than` and `less or equal than`.
However, if we allow `minInclusive / maxInclusive` customization (or the current lt/lte/gt/gte), we don't really need a counterpart `@Disabled...` annotation.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 13:06 ` "sehrope (@sehrope)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: sehrope (@sehrope) @ 2026-02-23 13:06 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
(on testkit/src/main/java/org/postgresql/test/annotations/EnabledForServerVersionRange.java:33)
No I don't like the hidden options with defaults as the entire purpose is to make the determination explicit. That's why I went with separate `lt` and `lte`. Ditto for not having a flag that flips it from "include" to "exclude".
The lt/lte or gt/gte check would be fine to add (the cross one is way more complicated).
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 13:07 ` "sehrope (@sehrope)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: sehrope (@sehrope) @ 2026-02-23 13:07 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Was mostly done addressing the feedback but didn't get a chance yet to finish testing the changes.
Snowed in for the day so figure will get to wrapping it up by this afternoon.
^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests
@ 2026-02-23 21:55 ` "sehrope (@sehrope)" <[email protected]>
14 siblings, 0 replies; 16+ messages in thread
From: sehrope (@sehrope) @ 2026-02-23 21:55 UTC (permalink / raw)
To: pgjdbc/pgjdbc <[email protected]>
Rebased and force pushed. Adds `@DisabledForServerVersionRange` as well (with no usage yet but ready if someone wants to use it).
Added a base parent for both as the majority of the version related code is the same. Each just defines how it matches and whether it's negative or positive result.
Also includes checks that:
* At least one of lt / lte / gte / gt is populated
* Both lt / lte cannot be populated
* Both gt / gte cannot be populated
To unify the code both are implemented by deciding if we match, and then returning either the enabled or disabled result depending on the annotation. Note that the "matches" logic for the disabled annotation is backwards. If it matches, the test is disabled.
The response message is slightly worse than before for the enabled annotation as we only say whether we did not match at all (previously we would explicitly say which of the lt / lte / gte / gt clauses was violated). But that was done because the same approach does not work for the disabled annotation and wanted to keep the code consistent. If someone is investigating can always look at the actual annotations anyway.
I'll merge this tomorrow if there's nothing new.
^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2026-02-23 21:55 UTC | newest]
Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-14 18:35 [pgjdbc/pgjdbc] PR #3939: Add RequireServerVersion annotation for tests "sehrope (@sehrope)" <[email protected]>
2026-02-14 18:48 ` "vlsi (@vlsi)" <[email protected]>
2026-02-14 21:40 ` "sehrope (@sehrope)" <[email protected]>
2026-02-14 22:04 ` "vlsi (@vlsi)" <[email protected]>
2026-02-15 11:13 ` "davecramer (@davecramer)" <[email protected]>
2026-02-16 15:33 ` "sehrope (@sehrope)" <[email protected]>
2026-02-16 16:18 ` "vlsi (@vlsi)" <[email protected]>
2026-02-23 10:55 ` "davecramer (@davecramer)" <[email protected]>
2026-02-23 11:49 ` "vlsi (@vlsi)" <[email protected]>
2026-02-23 11:50 ` "vlsi (@vlsi)" <[email protected]>
2026-02-23 11:50 ` "vlsi (@vlsi)" <[email protected]>
2026-02-23 12:02 ` "vlsi (@vlsi)" <[email protected]>
2026-02-23 12:05 ` "vlsi (@vlsi)" <[email protected]>
2026-02-23 13:06 ` "sehrope (@sehrope)" <[email protected]>
2026-02-23 13:07 ` "sehrope (@sehrope)" <[email protected]>
2026-02-23 21:55 ` "sehrope (@sehrope)" <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox