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

Clean up some tests and fix IsValidTest race condition #1581

Merged
merged 5 commits into from Oct 24, 2019

Conversation

@sehrope
Copy link
Contributor

sehrope commented Oct 12, 2019

This PR:

  • Adds some static helpers to TestUtil for retrieving and killing backend connections.
  • Cleans up IsValidTest to test/sleep in a loop after killing the connection to deal with the race condition that's periodically breaking testing. Also splits the transaction state tests into separate methods based upon the situation being tested.
  • Clean up CopyTest and ConnectionPoolTest to use the new helper. Also changes these to use a non-privileged connection as super user is not required for killing processes owned by the same user.
sehrope added 4 commits Oct 12, 2019
Adds some helpers to TestUtil to deal with boiler plate of executinga query
and disgarding the results, fetching the backend process id, terminating a
backend process, and fetching the current transaction state.
Cleans up IsValidTest and removes a race condition when checking whether
terminated connections are marked as invalid. Also splits out the transaction
state test into three separate ones to better identify what's being tested.
This also changes the connection termination to use a non-privileged
connection.
This also changes the pg_terminate_backend(...) call to use a non-privileged connection
as a regular user can terminate their own connection.
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Oct 12, 2019

Older versions of the server (<=9.1) restrict calling pg_terminate_backend(...)
to super users so always use that in test helper.
@AppVeyorBot

This comment has been minimized.

Copy link

AppVeyorBot commented Oct 12, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 13, 2019

Codecov Report

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

@@             Coverage Diff              @@
##             master    #1581      +/-   ##
============================================
+ Coverage     68.92%   68.96%   +0.04%     
- Complexity     3995     3997       +2     
============================================
  Files           179      179              
  Lines         16622    16622              
  Branches       2707     2707              
============================================
+ Hits          11457    11464       +7     
+ Misses         3908     3904       -4     
+ Partials       1257     1254       -3
@davecramer

This comment has been minimized.

Copy link
Member

davecramer commented Oct 13, 2019

@sehrope are you satisfied with this or do you have more work to do ?

@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Oct 13, 2019

I'm done with this PR and think it's good to go.

@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Oct 24, 2019

@davecramer Anything holding up merging this in?

@davecramer davecramer merged commit ad73457 into pgjdbc:master Oct 24, 2019
3 checks passed
3 checks passed
codecov/project 68.96% (+0.04%) compared to 69edc0b
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sehrope

This comment has been minimized.

Copy link
Contributor Author

sehrope commented Oct 24, 2019

Guess not :)

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