HBASE-29375 Add Unit Tests for BackupAdminImpl and Improve Test Granularity #7171

vinayakphegde · 2025-07-24T12:48:38Z

No description provided.

…ularity

Copilot

Pull Request Overview

This PR adds comprehensive unit tests for the BackupAdminImpl class in HBase's backup functionality, enhancing test coverage and granularity. The implementation follows best practices by using mocks to isolate the unit under test and making several private methods package-private to enable targeted testing.

Introduces a new test class TestBackupAdminImpl with 15 comprehensive test methods
Makes 6 private methods package-private using the @RestrictedApi annotation to enable testing
Adds extensive test coverage for backup deletion, merge validation, and cleanup operations

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
TestBackupAdminImpl.java	New comprehensive test class covering backup deletion, merge validation, and cleanup functionality with proper mocking
BackupAdminImpl.java	Modified visibility of 6 methods from private to package-private using @RestrictedApi annotations for test access

Comments suppressed due to low confidence (1)

hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/impl/TestBackupAdminImpl.java:98

The test creates a BackupInfo with default values (null fields) which may not accurately represent real backup data. Consider creating BackupInfo instances with realistic values to improve test validity.

    backupHistory.put(TableName.valueOf("ns:table1"), List.of(new BackupInfo())); // Only table1

Copilot · 2025-07-28T17:24:05Z

hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/impl/TestBackupAdminImpl.java

+    Path expectedPath = new Path(BackupUtils.getTableBackupDir(backupRootDir, backupId, table));
+
+    // Mock getFileSystem to return our mock FileSystem
+    doReturn(mockFs).when(backupAdminImpl).getFileSystem(any(Path.class), eq(conf));
+    when(mockFs.delete(expectedPath, true)).thenReturn(true);
+
+    // Call the method under test
+    backupAdminImpl.cleanupBackupDir(mockBackupInfo, table, conf);
+
+    // Verify the FileSystem delete call with correct path
+    verify(mockFs).delete(expectedPath, true);


The test relies on BackupUtils.getTableBackupDir() without mocking it, creating a dependency on the utility class behavior. Consider mocking this method or using a known path for better test isolation.

Suggested change

Path expectedPath = new Path(BackupUtils.getTableBackupDir(backupRootDir, backupId, table));

// Mock getFileSystem to return our mock FileSystem

doReturn(mockFs).when(backupAdminImpl).getFileSystem(any(Path.class), eq(conf));

when(mockFs.delete(expectedPath, true)).thenReturn(true);

// Call the method under test

backupAdminImpl.cleanupBackupDir(mockBackupInfo, table, conf);

// Verify the FileSystem delete call with correct path

verify(mockFs).delete(expectedPath, true);

try (MockedStatic<BackupUtils> mockedBackupUtils = mockStatic(BackupUtils.class)) {

Path expectedPath = new Path("/mocked/path/to/backup");

mockedBackupUtils.when(() -> BackupUtils.getTableBackupDir(backupRootDir, backupId, table))

.thenReturn(expectedPath.toString());

// Mock getFileSystem to return our mock FileSystem

doReturn(mockFs).when(backupAdminImpl).getFileSystem(any(Path.class), eq(conf));

when(mockFs.delete(expectedPath, true)).thenReturn(true);

// Call the method under test

backupAdminImpl.cleanupBackupDir(mockBackupInfo, table, conf);

// Verify the FileSystem delete call with correct path

verify(mockFs).delete(expectedPath, true);

}

Copilot · 2025-07-28T17:24:05Z

hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/impl/TestBackupAdminImpl.java

+
+    BackupInfo info = new BackupInfo();
+    info.setBackupId("backup_002");
+    info.setTables(new ArrayList<>(List.of(onlyTable)));


[nitpick] Creating an ArrayList from a List.of() is unnecessarily complex. Consider using 'new ArrayList<>()' followed by 'add()' or use 'Arrays.asList()' for better readability.

Suggested change

info.setTables(new ArrayList<>(List.of(onlyTable)));

info.setTables(new ArrayList<>(Arrays.asList(onlyTable)));

Copilot · 2025-07-28T17:24:06Z

hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/impl/TestBackupAdminImpl.java

+    BackupInfo dependent = new BackupInfo();
+    dependent.setBackupId("backup_inc_002");
+    dependent.setBackupRootDir("/backup/root");
+    dependent.setTables(new ArrayList<>(List.of(table)));


[nitpick] Creating an ArrayList from a List.of() is unnecessarily complex. Consider using 'new ArrayList<>()' followed by 'add()' or use 'Arrays.asList()' for better readability.

Suggested change

dependent.setTables(new ArrayList<>(List.of(table)));

dependent.setTables(new ArrayList<>(Arrays.asList(table)));

taklwu

LGTM

taklwu · 2025-07-28T20:26:16Z

hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/impl/TestBackupAdminImpl.java

+   * the current backup. Only backups affecting the specified table should be considered.
+   */
+  @Test
+  public void testGetAffectedBackupSessions_skipsNonMatchingTable() throws IOException {


nit: do we have a test for a other table with BackupType.FULL , and I assumed it's not going to include those unmatched table.

Yes, correct. I will add a test for that.

kgeisz

LGTM overall. I just have on question regarding the method documentation for testDeleteBackupWithBulkLoadedFiles().

kgeisz · 2025-07-28T22:23:41Z

hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/impl/TestBackupAdminImpl.java

+   * Tests that deleteBackup will remove bulk-loaded files and handle exceptions gracefully.
+   */
+  @Test
+  public void testDeleteBackupWithBulkLoadedFiles() throws Exception {


What exception gets thrown/handled in this test?

java.io.IOException. I will change the method signature to match that.

Apache-HBase · 2025-07-29T05:17:15Z

🎊 +1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 34s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
+1 💚	hbaseanti	0m 0s		Patch does not have any anti-patterns.
			_ HBASE-28957 Compile Tests _
+1 💚	mvninstall	3m 22s		HBASE-28957 passed
+1 💚	compile	0m 32s		HBASE-28957 passed
-0 ⚠️	checkstyle	0m 10s	/buildtool-branch-checkstyle-hbase-backup.txt	The patch fails to run checkstyle in hbase-backup
+1 💚	spotbugs	0m 32s		HBASE-28957 passed
+1 💚	spotless	0m 48s		branch has no errors when running spotless:check.
			_ Patch Compile Tests _
+1 💚	mvninstall	3m 12s		the patch passed
+1 💚	compile	0m 31s		the patch passed
-0 ⚠️	javac	0m 31s	/results-compile-javac-hbase-backup.txt	hbase-backup generated 1 new + 117 unchanged - 0 fixed = 118 total (was 117)
+1 💚	blanks	0m 0s		The patch has no blanks issues.
-0 ⚠️	checkstyle	0m 9s	/buildtool-patch-checkstyle-hbase-backup.txt	The patch fails to run checkstyle in hbase-backup
+1 💚	spotbugs	0m 36s		the patch passed
+1 💚	hadoopcheck	12m 11s		Patch does not cause any errors with Hadoop 3.3.6 3.4.0.
+1 💚	spotless	0m 47s		patch has no errors when running spotless:check.
			_ Other Tests _
+1 💚	asflicense	0m 10s		The patch does not generate ASF License warnings.
		31m 15s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7171/2/artifact/yetus-general-check/output/Dockerfile
GITHUB PR	#7171
Optional Tests	dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless
uname	Linux f7aa62b088d5 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	HBASE-28957 / `fdd5763`
Default Java	Eclipse Adoptium-17.0.11+9
Max. process+thread count	83 (vs. ulimit of 30000)
modules	C: hbase-backup U: hbase-backup
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7171/2/console
versions	git=2.34.1 maven=3.9.8 spotbugs=4.7.3
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

Apache-HBase · 2025-07-29T05:30:26Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 39s		Docker mode activated.
-0 ⚠️	yetus	0m 3s		Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck
			_ Prechecks _
			_ HBASE-28957 Compile Tests _
+1 💚	mvninstall	3m 16s		HBASE-28957 passed
+1 💚	compile	0m 19s		HBASE-28957 passed
+1 💚	javadoc	0m 14s		HBASE-28957 passed
+1 💚	shadedjars	5m 57s		branch has no errors when building our shaded downstream artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	3m 2s		the patch passed
+1 💚	compile	0m 19s		the patch passed
+1 💚	javac	0m 19s		the patch passed
+1 💚	javadoc	0m 13s		the patch passed
+1 💚	shadedjars	5m 53s		patch has no errors when building our shaded downstream artifacts.
			_ Other Tests _
-1 ❌	unit	23m 26s	/patch-unit-hbase-backup.txt	hbase-backup in the patch failed.
		44m 23s

Subsystem	Report/Notes
Docker	ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7171/2/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile
GITHUB PR	#7171
Optional Tests	javac javadoc unit compile shadedjars
uname	Linux e356b9eb0bc2 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/hbase-personality.sh
git revision	HBASE-28957 / `fdd5763`
Default Java	Eclipse Adoptium-17.0.11+9
Test Results	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7171/2/testReport/
Max. process+thread count	3629 (vs. ulimit of 30000)
modules	C: hbase-backup U: hbase-backup
Console output	https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-7171/2/console
versions	git=2.34.1 maven=3.9.8
Powered by	Apache Yetus 0.15.0 https://yetus.apache.org

This message was automatically generated.

…larity (apache#7171) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>

…larity (#7171) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>

HBASE-29375: Add Unit Tests for BackupAdminImpl and Improve Test Gran…

4876e89

…ularity

This comment has been minimized.

Sign in to view

taklwu requested a review from Copilot July 28, 2025 17:23

Copilot AI reviewed Jul 28, 2025

View reviewed changes

taklwu approved these changes Jul 28, 2025

View reviewed changes

kgeisz approved these changes Jul 28, 2025

View reviewed changes

add new unit tests

fdd5763

taklwu merged commit a962599 into apache:HBASE-28957 Jul 29, 2025
1 check failed

anmolnar pushed a commit that referenced this pull request Sep 11, 2025

HBASE-29375 Add Unit Tests for BackupAdminImpl and Improve Test Granu…

2ffedfd

…larity (#7171) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>

anmolnar pushed a commit that referenced this pull request Nov 6, 2025

HBASE-29375 Add Unit Tests for BackupAdminImpl and Improve Test Granu…

3044b11

…larity (#7171) Signed-off-by: Tak Lon (Stephen) Wu <taklwu@apache.org> Reviewed by: Kevin Geiszler <kevin.j.geiszler@gmail.com>

	info.setTables(new ArrayList<>(List.of(onlyTable)));
	info.setTables(new ArrayList<>(Arrays.asList(onlyTable)));

	dependent.setTables(new ArrayList<>(List.of(table)));
	dependent.setTables(new ArrayList<>(Arrays.asList(table)));

HBASE-29375 Add Unit Tests for BackupAdminImpl and Improve Test Granularity #7171

HBASE-29375 Add Unit Tests for BackupAdminImpl and Improve Test Granularity #7171

Uh oh!

Conversation

vinayakphegde commented Jul 24, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

taklwu left a comment

Choose a reason for hiding this comment

Uh oh!

taklwu Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

vinayakphegde Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

kgeisz left a comment

Choose a reason for hiding this comment

Uh oh!

kgeisz Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

vinayakphegde Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

Apache-HBase commented Jul 29, 2025

Uh oh!

Apache-HBase commented Jul 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants