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

test: assume integer datetimes for timestamp tests #873

Merged
merged 1 commit into from Jul 24, 2017

Conversation

Projects
None yet
3 participants
@grzm
Contributor

grzm commented Jul 22, 2017

Float timestamp equality comparisons like comparisons with any float
are problematic if they don't take into account their imprecise
nature. Some timestamp tests produce false negative failures for
servers compiled with float timestamps. Use JUnit assumptions to skip
these tests if the server doesn't have integer datetimes.

GetObject310Test and SetObject310Test still produce these false
negative failures. Failing assumptions in these tests throw
AssumptionViolationException rather than skipping the tests. This
may be due to these tests extending BaseTest which extends
junit.framework.TestCase rather than being treated as JUnit 4
tests. Leave these failing tests as-is for now as it's the existing
behavior, and throwing these exceptions exposes an issue with the test
setup rather than testing JDBC behavior.

This addresses #12 in part in
that it omits (most of) the failing tests, though it leaves these
timestamp behaviors untested under float timestamps, which was the
default prior to PostgreSQL 8.4.

@codecov-io

This comment has been minimized.

codecov-io commented Jul 22, 2017

Codecov Report

Merging #873 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master    #873      +/-   ##
===========================================
+ Coverage     65.69%   65.7%   +<.01%     
- Complexity     3544    3545       +1     
===========================================
  Files           166     166              
  Lines         15253   15253              
  Branches       2474    2474              
===========================================
+ Hits          10021   10022       +1     
  Misses         4052    4052              
+ Partials       1180    1179       -1
@grzm

This comment has been minimized.

Contributor

grzm commented Jul 22, 2017

I'm not sure how to interpret the code coverage reduction. It shows changes in files (PgStatement.java and CopyManager.java) not included in the request:

https://codecov.io/gh/pgjdbc/pgjdbc/compare/c1d743f2408df8b377bc9d8440717541a5b627e3...574daad2d2ccabf9a51b36de0af4716aa41fea52/changes

Adding the assumptions will in effect cause some branches to be skipped in some environments (after all, that's kind of the point of using assumptions), but it's not clear to me how this pull request affects these branches. Any guidance appreciated.

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 22, 2017

I'm not sure how to interpret the code coverage reduction

Same thing for me. I don't know how to setup codecov better.

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 22, 2017

Am I right this is to "heal" tests agains 8.2?

@grzm

This comment has been minimized.

Contributor

grzm commented Jul 22, 2017

I came across it when running tests for 8.3. I haven't run it against 8.2 locally, though I don't know why it wouldn't work there either. I'm not familiar with testing issues under 8.2.

@vlsi

This comment has been minimized.

Member

vlsi commented Jul 22, 2017

I came across it when running tests for 8.3. I haven't run it against 8.2 locally.

We have a Travis job testing against 8.2
And it looks like there are just two test left

Failed tests: 
  AutoRollbackTestSuite.run:256->executeSqlSuccess:323 autosave=NEVER, thus the transaction should be killed
Tests in error: 
  ResultSetTest.testgetBadBoolean:306->testBadBoolean:312 » PSQL ERROR: type "uu...
@grzm

This comment has been minimized.

Contributor

grzm commented Jul 22, 2017

I've added a pull request for the skipping UUID on 8.2. UUID was added in 8.3.

#874

@grzm

This comment has been minimized.

Contributor

grzm commented Jul 23, 2017

I've submitted a PR that fixes the ResultSetTest error in the 8.2 builds:

#876

@grzm

This comment has been minimized.

Contributor

grzm commented Jul 23, 2017

Regarding the coverage regression, I haven't been able to figure out the cause. From what I can tell, the Debian packages used in the Travis builds all include --enable-integer-datetimes, so the assumptions should have no effect on outcome of the tests at all--they should effectively be no-ops. It would make sense if they were skipping previously failing or erring tests, but that's not the case from what I can tell.

Regardless, it appears that the previous coverage was accidental rather than deliberate, so the regression may not be meaningful in the sense that no tests that were intended to exercise the code were removed. Unfortunately my understanding of PgStatement.java--and the branch at Line 903--isn't sufficient to provide either an explanation of what changed with the given code or what test would cover the line.

@vlsi vlsi added this to the 42.1.4 milestone Jul 23, 2017

@@ -660,6 +660,13 @@ public static boolean isProtocolVersion(Connection con, int version) {
return false;
}

public static boolean haveIntegerDateTimes(Connection con) {
if (con instanceof PgConnection) {

This comment has been minimized.

@vlsi

vlsi Jul 23, 2017

Member

Please add if (con == null) { throw new NullPointerException("Connection is null"); } kind of check to avoid silent failures.

@grzm grzm force-pushed the grzm:assume-integer-datetimes branch from 574daad to 2e53d5c Jul 23, 2017

@@ -664,6 +664,28 @@ public static boolean haveMinimumJVMVersion(String version) {
return (jvm.compareTo(version) >= 0);
}

@Deprecated
public static boolean isProtocolVersion(Connection con, int version) {

This comment has been minimized.

@vlsi

vlsi Jul 23, 2017

Member

This has just been removed (see 0221f93 ), please do not resurrect it.

test: assume integer datetimes for timestamp tests
Float timestamp equality comparisons like comparisons with any float
are problematic if they don't take into account their imprecise
nature. Some timestamp tests produce false negative failures for
servers compiled with float timestamps. Use JUnit assumptions to skip
these tests if the server doesn't have integer datetimes.

This addresses #12 in part in
that it omits (most of) the failing tests, though it leaves these
timestamp behaviors untested under float timestamps, which was the
default prior to PostgreSQL 8.4.

@grzm grzm force-pushed the grzm:assume-integer-datetimes branch from 3dfe99b to 1e519ba Jul 23, 2017

@vlsi

vlsi approved these changes Jul 24, 2017

@vlsi vlsi merged commit 8287e7f into pgjdbc:master Jul 24, 2017

2 checks passed

codecov/project 65.7% (+<.01%) compared to ed0014c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@grzm

This comment has been minimized.

Contributor

grzm commented Jul 24, 2017

Thanks!

@grzm grzm deleted the grzm:assume-integer-datetimes branch Jul 24, 2017

davecramer added a commit to davecramer/pgjdbc that referenced this pull request Sep 19, 2017

test: assume integer datetimes for timestamp tests (pgjdbc#873)
Float timestamp equality comparisons like comparisons with any float
are problematic if they don't take into account their imprecise
nature. Some timestamp tests produce false negative failures for
servers compiled with float timestamps. Use JUnit assumptions to skip
these tests if the server doesn't have integer datetimes.

This addresses pgjdbc#12 in part in
that it omits (most of) the failing tests, though it leaves these
timestamp behaviors untested under float timestamps, which was the
default prior to PostgreSQL 8.4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment