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

refactor: migrate to Junit4 #738

Closed
wants to merge 21 commits into from
Closed

Conversation

@AlexElin
Copy link
Contributor

@AlexElin AlexElin commented Jan 22, 2017

migrate tests to Junit4

#205

AlexElin added 4 commits Jan 22, 2017
migrate tests to Junit4

#205
checkstyle

#205
fix compilation

#205
fix compilation

#205
@codecov-io
Copy link

@codecov-io codecov-io commented Jan 23, 2017

Codecov Report

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

@@             Coverage Diff              @@
##             master     #738      +/-   ##
============================================
+ Coverage     65.57%   65.58%   +0.01%     
- Complexity     3549     3550       +1     
============================================
  Files           166      166              
  Lines         15254    15254              
  Branches       2475     2475              
============================================
+ Hits          10003    10005       +2     
+ Misses         4070     4069       -1     
+ Partials       1181     1180       -1
Copy link
Member

@vlsi vlsi left a comment

Not sure if usage of BaseTest is compatible with JUnit 4.

I think BaseTest should be replaced with BaseTest4, and BaseTest shold be dropped to prevent it from being used in the future.

@@ -9,10 +9,6 @@

public class GetObject310BinaryTest extends GetObject310Test {

public GetObject310BinaryTest(String name) {

This comment has been minimized.

@vlsi

vlsi Jan 24, 2017
Member

Do GetObject310Test and GetObject310BinaryTest work after your changes?

Previously GetObject310Test was powered by extends BaseTest, and now you seem to convert it to JUnit4 format.
That means updateProperties will never be called. Will it?

This comment has been minimized.

@AlexElin

AlexElin Feb 7, 2017
Author Contributor

It would be called.

I removed the GetObject310BinaryTest and made the GetObject310Test as parameterized.

AlexElin added 13 commits Feb 3, 2017
replace BaseTest with BaseTest4
parameterize GetObject310Test and UpsertTest

#205
parameterize CursorFetchTest

#205
checkstyle

#205
migrate StringTypeParameterTest and Jdbc3TestSuite

#205
# Conflicts:
#	pgjdbc/src/test/java/org/postgresql/test/jdbc42/Jdbc42TestSuite.java
use JUnit 4 in the README for tests

#205
fix checkstyle

#205
remove unused BaseTest class and use BaseTest4 in some classes

#205
fix isValidTest

#205
migrate Jdbc2TestSuite.java to JUnit4

#205
@davecramer
Copy link
Member

@davecramer davecramer commented May 19, 2017

Is this ready to go ?

public void testParameterStringTypeNotSet() throws Exception {
if (!TestUtil.isProtocolVersion(_conn, 3)) {
return;
}
testParameterVarchar(null);
}

@Test
public void testParameterUnspecified() throws Exception {

This comment has been minimized.

@vlsi

vlsi May 19, 2017
Member

This looks like a code movement. Could you please move the method back?

This comment has been minimized.

@AlexElin

AlexElin May 19, 2017
Author Contributor

done

}

protected boolean setUp(String stringType) throws Exception {
private boolean prepare(String stringType) throws Exception {

This comment has been minimized.

@vlsi

vlsi May 19, 2017
Member

This might be a case for @Parameterized. On the other hand, I'm fine to keep the test as is.

This comment has been minimized.

@AlexElin

AlexElin Jun 28, 2017
Author Contributor

two of three test methods do the same checks, but the third does not do all checks that others do.
Let's keep the test as is

This comment has been minimized.

@vlsi

vlsi Jul 23, 2017
Member

The replacement of setUp with prepare does break things.

For instance, testParameterStringTypeVarchar has the following code:

  @Test
  public void testParameterStringTypeVarchar() throws Exception {
    if (!TestUtil.isProtocolVersion(_conn, 3)) {
      return;
    }
    testParameterVarchar("varchar");
  }

The thing is TestUtil.isProtocolVersion check should be performed when connection is active. When setUp got replaced with prepare, the connection became null, and isProtocolVersion became always false.

This comment has been minimized.

@AlexElin

AlexElin Jul 23, 2017
Author Contributor

I think this behavoiur had been before I replaced setUp with prepare. Because 'setUp' had parameter, JUnit didn't pick up it (http://junit.sourceforge.net/junit3.8.1/javadoc/junit/framework/TestCase.html#setUp()).

This comment has been minimized.

@AlexElin

AlexElin Jul 23, 2017
Author Contributor

I think it's worth to remove checks TestUtil.isProtocolVersion(_conn, 3) because this driver is "compatible with PostgreSQL 8.2 and higher using the version 3.0 of the protocol". Therefore this check always returns true

This comment has been minimized.

@AlexElin

AlexElin Jul 23, 2017
Author Contributor

@vlsi do you have objections againts removing this checks?

This comment has been minimized.

@vlsi

vlsi Jul 23, 2017
Member

The test indeed was broken long ago. I will refactor it to junit4

AlexElin added 4 commits May 19, 2017
This reverts commit c5cd6d7
revert method moving

#205
vlsi added a commit to vlsi/pgjdbc that referenced this pull request Jul 23, 2017
fixes pgjdbc#738
fixes pgjdbc#205
@vlsi vlsi closed this in 5b65e2f Jul 23, 2017
@vlsi vlsi added this to the 42.1.4 milestone Jul 23, 2017
@vlsi
Copy link
Member

@vlsi vlsi commented Jul 23, 2017

@AlexElin , thanks for the migration.

@AlexElin AlexElin deleted the AlexElin:migrate_to_junit4 branch Jul 24, 2017
davecramer added a commit to davecramer/pgjdbc that referenced this pull request Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.