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

Fix TestUtil.dropXyz(...) object not exists errors #1359

Merged
merged 5 commits into from Dec 5, 2018

Conversation

Projects
None yet
4 participants
@sehrope
Copy link
Contributor

commented Nov 28, 2018

  • - Tests pass
  • - Checkstyle pass

While testing some update to a VM for testing pgjdbc I noticed a bunch of errors in the server error logs for trying to drop missing objects as the test SQL was not using ... IF EXISTS ....

The first commit in the PR changes a bunch of the dropXYZ(...) methods in TestUtil to use ... DROP EXISTS ... if the connection is not in a transaction. This matches the behavior in the comments around the drop statements. This also changes the method to not skip over non-transactional exceptions in the drop statements that are not due to missing objects such as a connection error or a dependency on the dropped object.

That last one (dependencies) lead to a bunch of broken tests that were previously passing because they were ignoring those errors. Here's an example:

testForeignKeysToUniqueIndexes(org.postgresql.test.jdbc2.DatabaseMetaDataTest)  Time elapsed: 0.017 sec  <<< ERROR!
org.postgresql.util.PSQLException: ERROR: cannot drop sequence sercoltest_b_seq because other objects depend on it
  Detail: default for table sercoltest column b depends on sequence sercoltest_b_seq
  Hint: Use DROP ... CASCADE to drop the dependent objects too.
	at org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2440)
	at org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2183)
	at org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:308)
	at org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:441)
	at org.postgresql.jdbc.PgStatement.execute(PgStatement.java:365)
	at org.postgresql.jdbc.PgStatement.executeWithFlags(PgStatement.java:307)
	at org.postgresql.jdbc.PgStatement.executeCachedSql(PgStatement.java:293)
	at org.postgresql.jdbc.PgStatement.executeWithFlags(PgStatement.java:270)
	at org.postgresql.jdbc.PgStatement.executeUpdate(PgStatement.java:244)
	at org.postgresql.test.TestUtil.dropSequence(TestUtil.java:524)
	at org.postgresql.test.jdbc2.DatabaseMetaDataTest.setUp(DatabaseMetaDataTest.java:48)

The rest of the commits fix the dependency related drop errors. This is done through either adding missing ... CASCADE ... clauses or explicitly ordering the drops so they happen in the right order.

@codecov-io

This comment has been minimized.

Copy link

commented Nov 28, 2018

Codecov Report

Merging #1359 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1359      +/-   ##
============================================
+ Coverage     68.65%   68.67%   +0.01%     
- Complexity     3889     3890       +1     
============================================
  Files           179      179              
  Lines         16391    16391              
  Branches       2669     2669              
============================================
+ Hits          11253    11256       +3     
+ Misses         3888     3886       -2     
+ Partials       1250     1249       -1
@vlsi

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

Just wondering: should we always use if not exist variation?

@sehrope

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2018

I thought about that as well but if a test is doing something like:

  1. CREATE ...
  2. Do stuff...
  3. DROP ...
  4. More stuff ...

... and all of that is happening in a transaction then I'd consider it a failure if the DROP didn't actually drop something. That's how the previous code handled that situation and what the comments to the code said it was supposed to do.

sehrope added some commits Nov 28, 2018

test: Change drop object utilities in TestUtils to use IF EXISTS
Changes most drop object helper methods in TestUtils to check if a transaction
is in progress and, if not, then use IF EXISTS to suppress errors for missing
objects during the DROP command.

If a transaction is in progress then the command then the drop is executed
without the IF EXISTS clause so that missing objects results in an exception
and test failure.

In either case an unrelated error such as the server being offline will be now
throw an exception. This is a change from the prior behavior where all errors
that are not part of a transaction were being suppressed.
errors if the object being dropped does not exist.
test: Change TestUtil.dropSequence(...) to add CASCADE
Adds CASCADE option to TestUtils.dropSequence(...) helper to handle
dependency errors in drop statements that previously being ignored.
test: Add shouldDrop param to TestUtils.createCompositeType(...)
Also adds a method matching the previous signature without the new parameter
and defaulting to the older behavior of true (i.e. drop then create).
test: Use explicit drop type statements for metadata tests
Adds explicit drop statements for types in metadata tests to ensure
the drop statements are executed in the correct order. Also adds a
drop statement for the reference table.

The revised drop order is required as TestUtils no longer skips over
non-missing object errors when dropping objects.
test: Clean up throws clauses in TestUtil
Restricts throws clauses in TestUtil to SQLException rather than a
more broad Exception and removes throws clause from initDriver().

@sehrope sehrope force-pushed the sehrope:fix-test-util-drop-errors branch from ac5429e to 590823d Dec 4, 2018

@sehrope

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Rebased atop master and added one more commit that cleans up some of the throws Exception clauses in TestUtil to either remove them entirely (if the method doesn't actually throw anything) or restrict to a more specific SQLException.

@davecramer

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

LGTM

@davecramer davecramer merged commit 0999bb7 into pgjdbc:master Dec 5, 2018

1 of 2 checks passed

codecov/project 68.65% (-0.02%) compared to 5b0c05f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sehrope sehrope deleted the sehrope:fix-test-util-drop-errors branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.