Skip to content

HBASE-22382 Refactor tests in TestFromClientSide #385

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.

Sign up for GitHub

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

Merged
Apache9 merged 4 commits into from
Jul 24, 2019
Merged

HBASE-22382 Refactor tests in TestFromClientSide #385

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f2d99cc
HBASE-22382. Refactored testNull() into multiple tests
anmolnar Jul 15, 2019
8536682
HBASE-22382. Suppress checkstyle warning on testVersionLimits() and t…
anmolnar Jul 15, 2019
dfe1d5b
HBASE-22382. Change test naming to camel case
anmolnar Jul 19, 2019
ddcd7ef
HBASE-22382. Refactored test to take advantage of JUnit expected exce…
anmolnar Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1156,20 +1156,28 @@ public void testSingleRowMultipleFamily() throws Exception {
}

@Test
public void testNull() throws Exception {
final TableName tableName = TableName.valueOf(name.getMethodName());

public void testNullTableName() {
// Null table name (should NOT work)
try {
TEST_UTIL.createTable((TableName)null, FAMILY);
fail("Creating a table with null name passed, should have failed");
} catch(Exception e) {}
Copy link
Contributor

@Apache9 Apache9 Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have split the method, we can use the @test(expected = XXXException.class) so we do not need to catch the exception any more?

Copy link
Contributor Author

@anmolnar anmolnar Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Makes perfect sense.

}

@Test
public void testNullFamilyName() {
final TableName tableName = TableName.valueOf(name.getMethodName());

// Null family (should NOT work)
try {
TEST_UTIL.createTable(tableName, new byte[][]{null});
fail("Creating a table with a null family passed, should fail");
} catch(Exception e) {}
}

@Test
public void testNullRowAndQualifier() throws Exception {
final TableName tableName = TableName.valueOf(name.getMethodName());

try (Table ht = TEST_UTIL.createTable(tableName, FAMILY)) {

Expand Down Expand Up @@ -1201,9 +1209,13 @@ public void testNull() throws Exception {
assertEmptyResult(result);
}
}
}

// Use a new table
try (Table ht = TEST_UTIL.createTable(TableName.valueOf(name.getMethodName() + "2"), FAMILY)) {
@Test
public void testNullEmptyQualifier() throws Exception {
final TableName tableName = TableName.valueOf(name.getMethodName());

try (Table ht = TEST_UTIL.createTable(tableName, FAMILY)) {

// Empty qualifier, byte[0] instead of null (should work)
try {
Expand Down Expand Up @@ -1234,7 +1246,14 @@ public void testNull() throws Exception {
} catch (Exception e) {
throw new IOException("Using a row with null qualifier threw exception, should ");
Copy link
Contributor

@Apache9 Apache9 Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not your fault but since this is a refactoring, let's fix the message?

Copy link
Contributor Author

@anmolnar anmolnar Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, fixed.

}
}
}

@Test
public void testNullValue() throws IOException {
final TableName tableName = TableName.valueOf(name.getMethodName());

try (Table ht = TEST_UTIL.createTable(tableName, FAMILY)) {
// Null value
try {
Put put = new Put(ROW);
Expand Down Expand Up @@ -1550,6 +1569,7 @@ public void testVersions() throws Exception {
}

@Test
@SuppressWarnings("checkstyle:MethodLength")
Copy link
Contributor

@Apache9 Apache9 Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in the future we will also split these methods so let's do not suppress the warning?

Copy link
Contributor Author

@anmolnar anmolnar Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to do so. But it seems to me that these tests are doing a sequence of operations which cannot be broken without doing a lot of redundant initialisation logic. I believe that the gain of making these tests smaller doesn't overcome the hit on the efficiency.

To be honest, checkstyle rule for maximizing the size of integration tests doesn't make sense to me.

public void testVersionLimits() throws Exception {
final TableName tableName = TableName.valueOf(name.getMethodName());
byte [][] FAMILIES = makeNAscii(FAMILY, 3);
Expand Down Expand Up @@ -5965,6 +5985,7 @@ public void testNullWithReverseScan() throws Exception {
}

@Test
@SuppressWarnings("checkstyle:MethodLength")
Copy link
Contributor

@Apache9 Apache9 Jul 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

public void testDeletesWithReverseScan() throws Exception {
final TableName tableName = TableName.valueOf(name.getMethodName());
byte[][] ROWS = makeNAscii(ROW, 6);
Expand Down