ATLAS-5068: Improve Unit Test Coverage for Hive-bridge Module - #425
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds comprehensive unit test coverage for the Hive-bridge module, implementing test classes for core event handling functionality. The goal is to improve test coverage by adding unit tests for the most critical classes in the hive-bridge module.
- Added unit tests for 6 core Hive event classes including DropTable, DropDatabase, CreateTable, CreateHiveProcess, CreateDatabase, and BaseHiveEvent
- Implemented comprehensive test scenarios covering constructor validation, notification message generation, entity processing, and edge cases
- Added helper methods and mock setups to support complex testing scenarios with proper isolation
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| DropTableTest.java | Tests for table deletion events including metastore and non-metastore hook scenarios, entity qualification, and integration workflows |
| DropDatabaseTest.java | Tests for database deletion events covering both metastore and hive entity processing paths with various edge cases |
| CreateTableTest.java | Comprehensive tests for table creation events including DDL operations, external tables, HBase integration, and reflection-based private method testing |
| CreateHiveProcessTest.java | Tests for Hive process creation covering lineage processing, column lineage, DDL operations, and complex workflow scenarios |
| CreateDatabaseTest.java | Tests for database creation events including metastore hook scenarios and entity generation |
| BaseHiveEventTest.java | Extensive tests for the base event class covering utility methods, entity processing, qualified name generation, and constant validation |
| verify(context, times(1)).removeFromKnownTable(any(String.class)); | ||
| } | ||
|
|
||
| /* @Test |
There was a problem hiding this comment.
Remove commented-out test method or uncomment and implement it properly. Commented code reduces maintainability.
| /* @Test | |
| @Test |
| assertEquals(1, notifications.size()); | ||
| assertTrue(notifications.get(0) instanceof EntityDeleteRequestV2); | ||
|
|
||
| // verify(context).removeFromKnownDatabase(anyString()); |
There was a problem hiding this comment.
Remove commented-out verification code or uncomment it if the verification is needed for the test.
| // verify(context).removeFromKnownDatabase(anyString()); | |
| verify(context).removeFromKnownDatabase(anyString()); |
| } | ||
|
|
||
| // =================== getHiveMetastoreEntities Tests =================== | ||
| /* @Test |
There was a problem hiding this comment.
Multiple commented-out test methods should be removed or properly implemented. This reduces code maintainability and can cause confusion.
| // So we should get null result due to skipProcess logic | ||
| AssertJUnit.assertNull("Entities should be null for self lineage scenario", entities); | ||
| }*/ | ||
|
|
There was a problem hiding this comment.
Large block of commented-out test code should be removed to improve maintainability.
| import org.apache.atlas.type.AtlasTypeUtil; | ||
| import org.apache.hadoop.hive.metastore.IHMSHandler; | ||
| import org.apache.hadoop.hive.metastore.api.Database; | ||
| //import org.apache.hadoop.hive.metastore.api.DatabaseType; |
There was a problem hiding this comment.
Remove unused commented import statement.
| //import org.apache.hadoop.hive.metastore.api.DatabaseType; |
| import java.util.*; | ||
|
|
||
| import static org.mockito.Mockito.*; | ||
| import static org.junit.Assert.assertNotNull; |
There was a problem hiding this comment.
[nitpick] Mixed usage of TestNG and JUnit assertions. The file primarily uses TestNG's AssertJUnit but also imports JUnit's Assert. Consider standardizing on one assertion library for consistency.
| import static org.junit.Assert.assertNotNull; |
| when(database.getOwnerType()).thenReturn(PrincipalType.USER); | ||
|
|
||
| when(database.getCatalogName()).thenReturn("hive"); | ||
| // when(database.getCreateTime()).thenReturn(1753349206); |
There was a problem hiding this comment.
Remove commented-out mock setup code or uncomment it if needed for the test.
| // when(database.getCreateTime()).thenReturn(1753349206); |
|
|
||
|
|
||
|
|
||
| /* @Test |
There was a problem hiding this comment.
Multiple large blocks of commented-out test methods should be removed to improve code maintainability.
|
@aditya-gupta36 - the build fails with the following error. Please review and update. |
|
@mneethiraj Sir, Thanks for Feedback and update. Will look into it. |
11d0f09 to
724e163
Compare
724e163 to
ee25388
Compare
(cherry picked from commit 0e87796)
What changes were proposed in this pull request?
Added Unit TCs for Hive module
How was this patch tested?