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

HADOOP-19425. [JDK17] Upgrade JUnit from 4 to 5 in hadoop-azure Part1. #7369

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,17 @@
import java.io.IOException;

import org.apache.hadoop.conf.Configuration;
import org.junit.After;
import org.junit.Before;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.TestInfo;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import org.apache.hadoop.fs.Path;
import org.apache.hadoop.fs.azure.integration.AzureTestConstants;
import org.apache.hadoop.io.IOUtils;

import static org.junit.Assume.assumeNotNull;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
import static org.apache.hadoop.fs.azure.integration.AzureTestUtils.*;

/**
Expand All @@ -49,16 +50,16 @@ public abstract class AbstractWasbTestBase extends AbstractWasbTestWithTimeout
protected NativeAzureFileSystem fs;
protected AzureBlobStorageTestAccount testAccount;

@Before
public void setUp() throws Exception {
@BeforeEach
public void setUp(TestInfo testInfo) throws Exception {
AzureBlobStorageTestAccount account = createTestAccount();
assumeNotNull("test account", account);
assumeTrue(account != null, "test account");
bindToTestAccount(account);
}

@After
public void tearDown() throws Exception {
describe("closing test account and filesystem");
@AfterEach
public void tearDown(TestInfo info) throws Exception {
describe(info, "closing test account and filesystem");
testAccount = cleanupTestAccount(testAccount);
IOUtils.closeStream(fs);
fs = null;
Expand Down Expand Up @@ -146,31 +147,37 @@ protected Path path(String filepath) throws IOException {
/**
* Return a path bonded to this method name, unique to this fork during
* parallel execution.
* @param testInfo Provides information about the currently executing test method.
* This can include details such as the name of the test method, display name.
* @return a method name unique to (fork, method).
* @throws IOException IO problems
*/
protected Path methodPath() throws IOException {
return path(methodName.getMethodName());
protected Path methodPath(TestInfo testInfo) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is Junit 5 trying to ruin our lives. Everything worked.

setup should be caching that testinfo for the current suite, and all these helper methods just using that to derive values. we shouldn't be passing it down

Copy link
Contributor Author

@slfan1989 slfan1989 Feb 12, 2025

Choose a reason for hiding this comment

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

Thank you for raising this issue! In JUnit 5, org.junit.rules.TestName has been removed, and it is recommended to use TestInfo to get the test method name. TestInfo can be used as a parameter, applied in setup or test methods, but it cannot be declared as a class-level variable.

Your suggestion made me think of a new solution: we can implement a method to retrieve the test method name and register it using the @RegisterExtension annotation. This method will retrieve the current test method name during the execution of @BeforeEach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cnauroth @anujmodi2021 @steveloughran I have improved the code again. Could you please help review this PR?
Thank you very much!

return path(testInfo.getDisplayName());
}

/**
* Return a blob path bonded to this method name, unique to this fork during
* parallel execution.
* @param testInfo Provides information about the currently executing test method.
* This can include details such as the name of the test method, display name.
* @return a method name unique to (fork, method).
* @throws IOException IO problems
*/
protected Path methodBlobPath() throws IOException {
return blobPath(methodName.getMethodName());
protected Path methodBlobPath(TestInfo testInfo) throws IOException {
return blobPath(testInfo.getDisplayName());
}

/**
* Describe a test in the logs.
* @param testInfo Provides information about the currently executing test method.
* This can include details such as the name of the test method, display name.
* @param text text to print
* @param args arguments to format in the printing
*/
protected void describe(String text, Object... args) {
protected void describe(TestInfo testInfo, String text, Object... args) {
LOG.info("\n\n{}: {}\n",
methodName.getMethodName(),
testInfo.getDisplayName(),
String.format(text, args));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,48 +18,36 @@

package org.apache.hadoop.fs.azure;

import org.junit.Assert;
import org.junit.Before;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.rules.TestName;
import org.junit.rules.Timeout;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.TestInfo;
import org.junit.jupiter.api.Timeout;

import org.apache.hadoop.fs.azure.integration.AzureTestConstants;

/**
* Base class for any Wasb test with timeouts & named threads.
* This class does not attempt to bind to Azure.
*/
public class AbstractWasbTestWithTimeout extends Assert {

/**
* The name of the current method.
*/
@Rule
public TestName methodName = new TestName();
/**
* Set the timeout for every test.
* This is driven by the value returned by {@link #getTestTimeoutMillis()}.
*/
@Rule
public Timeout testTimeout = new Timeout(getTestTimeoutMillis());
@Timeout(AzureTestConstants.AZURE_TEST_TIMEOUT)
public class AbstractWasbTestWithTimeout extends Assertions {

/**
* Name the junit thread for the class. This will overridden
* before the individual test methods are run.
*/
@BeforeClass
@BeforeAll
public static void nameTestThread() {
Thread.currentThread().setName("JUnit");
}

/**
* Name the thread to the current test method.
*/
@Before
public void nameThread() {
Thread.currentThread().setName("JUnit-" + methodName.getMethodName());
@BeforeEach
public void nameThread(TestInfo testInfo) {
Thread.currentThread().setName("JUnit-" + testInfo.getDisplayName());
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.microsoft.azure.storage.*;
import com.microsoft.azure.storage.blob.*;
import com.microsoft.azure.storage.core.Base64;
import org.junit.Assert;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -50,6 +49,7 @@
import static org.apache.hadoop.fs.azure.AzureNativeFileSystemStore.KEY_USE_LOCAL_SAS_KEY_MODE;
import static org.apache.hadoop.fs.azure.AzureNativeFileSystemStore.KEY_USE_SECURE_MODE;
import static org.apache.hadoop.fs.azure.integration.AzureTestUtils.verifyWasbAccountNameInConfig;
import static org.junit.jupiter.api.Assertions.assertNotNull;

/**
* Helper class to create WASB file systems backed by either a mock in-memory
Expand Down Expand Up @@ -212,9 +212,9 @@ public Number getLatestMetricValue(String metricName, Number defaultValue)
* @return
*/
private boolean wasGeneratedByMe(MetricsRecord currentRecord) {
Assert.assertNotNull("null filesystem", fs);
Assert.assertNotNull("null filesystemn instance ID",
fs.getInstrumentation().getFileSystemInstanceId());
assertNotNull(fs, "null filesystem");
assertNotNull(fs.getInstrumentation().getFileSystemInstanceId(),
"null filesystemn instance ID");
String myFsId = fs.getInstrumentation().getFileSystemInstanceId().toString();
for (MetricsTag currentTag : currentRecord.tags()) {
if (currentTag.name().equalsIgnoreCase("wasbFileSystemId")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import java.io.OutputStream;
import java.util.Arrays;

import org.junit.Test;
import org.junit.jupiter.api.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import com.microsoft.azure.storage.OperationContext;
import com.microsoft.azure.storage.SendingRequestEvent;
import com.microsoft.azure.storage.StorageEvent;
import org.junit.Test;
import org.junit.jupiter.api.Test;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileStatus;
Expand All @@ -41,7 +41,7 @@

import static org.apache.hadoop.fs.azure.AzureNativeFileSystemStore.NO_ACCESS_TO_CONTAINER_MSG;
import static org.apache.hadoop.test.LambdaTestUtils.intercept;
import static org.junit.Assume.assumeNotNull;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

/**
* Error handling.
Expand Down Expand Up @@ -76,7 +76,7 @@ public void testAccessUnauthorizedPublicContainer() throws Exception {
try {
FileSystem.get(noAccessPath.toUri(), new Configuration())
.open(noAccessPath);
assertTrue("Should've thrown.", false);
assertTrue(false, "Should've thrown.");
} catch (AzureException ex) {
GenericTestUtils.assertExceptionContains(
String.format(NO_ACCESS_TO_CONTAINER_MSG, account, container), ex);
Expand Down Expand Up @@ -157,7 +157,7 @@ public void testTransientErrorOnDelete() throws Exception {
// Need to do this test against a live storage account
AzureBlobStorageTestAccount testAccount =
AzureBlobStorageTestAccount.create();
assumeNotNull(testAccount);
assumeTrue(testAccount != null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: would it be better to define a accessible method assumeNotNull(variable) that internally calls assumeTrue(variable!= null);
This will reduce the overall diffs I feel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your suggestions! I will try to improve this part of the code.

try {
NativeAzureFileSystem fs = testAccount.getFileSystem();
injectTransientError(fs, new ConnectionRecognizer() {
Expand Down Expand Up @@ -200,7 +200,7 @@ public void testTransientErrorOnCommitBlockList() throws Exception {
// Need to do this test against a live storage account
AzureBlobStorageTestAccount testAccount =
AzureBlobStorageTestAccount.create();
assumeNotNull(testAccount);
assumeTrue(testAccount != null);
try {
NativeAzureFileSystem fs = testAccount.getFileSystem();
injectTransientError(fs, new ConnectionRecognizer() {
Expand All @@ -224,7 +224,7 @@ public void testTransientErrorOnRead() throws Exception {
// Need to do this test against a live storage account
AzureBlobStorageTestAccount testAccount =
AzureBlobStorageTestAccount.create();
assumeNotNull(testAccount);
assumeTrue(testAccount != null);
try {
NativeAzureFileSystem fs = testAccount.getFileSystem();
Path testFile = new Path("/a/b");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@

import static org.apache.hadoop.fs.azure.AzureNativeFileSystemStore.KEY_CHECK_BLOCK_MD5;
import static org.apache.hadoop.fs.azure.AzureNativeFileSystemStore.KEY_STORE_BLOB_MD5;
import static org.junit.Assume.assumeNotNull;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

import java.io.ByteArrayInputStream;
import java.io.IOException;
Expand All @@ -35,8 +35,9 @@
import org.apache.hadoop.fs.azure.AzureNativeFileSystemStore.TestHookOperationContext;
import org.apache.hadoop.fs.azure.integration.AzureTestUtils;

import org.junit.After;
import org.junit.Test;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInfo;

import com.microsoft.azure.storage.Constants;
import com.microsoft.azure.storage.OperationContext;
Expand All @@ -56,7 +57,7 @@
public class ITestBlobDataValidation extends AbstractWasbTestWithTimeout {
private AzureBlobStorageTestAccount testAccount;

@After
@AfterEach
public void tearDown() throws Exception {
testAccount = AzureTestUtils.cleanupTestAccount(testAccount);
}
Expand All @@ -65,21 +66,21 @@ public void tearDown() throws Exception {
* Test that by default we don't store the blob-level MD5.
*/
@Test
public void testBlobMd5StoreOffByDefault() throws Exception {
public void testBlobMd5StoreOffByDefault(TestInfo testInfo) throws Exception {
testAccount = AzureBlobStorageTestAccount.create();
testStoreBlobMd5(false);
testStoreBlobMd5(false, testInfo);
}

/**
* Test that we get blob-level MD5 storage and validation if we specify that
* in the configuration.
*/
@Test
public void testStoreBlobMd5() throws Exception {
public void testStoreBlobMd5(TestInfo testInfo) throws Exception {
Configuration conf = new Configuration();
conf.setBoolean(KEY_STORE_BLOB_MD5, true);
testAccount = AzureBlobStorageTestAccount.create(conf);
testStoreBlobMd5(true);
testStoreBlobMd5(true, testInfo);
}

/**
Expand All @@ -91,12 +92,12 @@ private static String trim(String s, String toTrim) {
toTrim);
}

private void testStoreBlobMd5(boolean expectMd5Stored) throws Exception {
assumeNotNull(testAccount);
private void testStoreBlobMd5(boolean expectMd5Stored, TestInfo testInfo) throws Exception {
assumeTrue(testAccount != null);
// Write a test file.
NativeAzureFileSystem fs = testAccount.getFileSystem();
Path testFilePath = AzureTestUtils.pathForTests(fs,
methodName.getMethodName());
testInfo.getDisplayName());
String testFileKey = trim(testFilePath.toUri().getPath(), "/");
OutputStream outStream = fs.create(testFilePath);
outStream.write(new byte[] { 5, 15 });
Expand All @@ -109,7 +110,7 @@ private void testStoreBlobMd5(boolean expectMd5Stored) throws Exception {
if (expectMd5Stored) {
assertNotNull(obtainedMd5);
} else {
assertNull("Expected no MD5, found: " + obtainedMd5, obtainedMd5);
assertNull(obtainedMd5, "Expected no MD5, found: " + obtainedMd5);
}

// Mess with the content so it doesn't match the MD5.
Expand Down Expand Up @@ -137,8 +138,8 @@ private void testStoreBlobMd5(boolean expectMd5Stored) throws Exception {
}
StorageException cause = (StorageException)ex.getCause();
assertNotNull(cause);
assertEquals("Unexpected cause: " + cause,
StorageErrorCodeStrings.INVALID_MD5, cause.getErrorCode());
assertEquals(StorageErrorCodeStrings.INVALID_MD5, cause.getErrorCode(),
"Unexpected cause: " + cause);
}
}

Expand Down Expand Up @@ -192,7 +193,7 @@ private void checkObtainedMd5(String obtainedMd5) {
if (expectMd5) {
assertNotNull(obtainedMd5);
} else {
assertNull("Expected no MD5, found: " + obtainedMd5, obtainedMd5);
assertNull(obtainedMd5, "Expected no MD5, found: " + obtainedMd5);
}
}

Expand All @@ -211,7 +212,7 @@ private static boolean isGetRange(HttpURLConnection connection) {

private void testCheckBlockMd5(final boolean expectMd5Checked)
throws Exception {
assumeNotNull(testAccount);
assumeTrue(testAccount != null);
Path testFilePath = new Path("/testFile");

// Add a hook to check that for GET/PUT requests we set/don't set
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import java.util.Arrays;
import java.util.Date;

import org.junit.Test;
import org.junit.jupiter.api.Test;

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileSystem;
Expand Down
Loading