[#7280] refactor(UT): localize schema stubbing in TestHadoopCatalogOperations #7335
Conversation
| @@ -610,6 +606,7 @@ public void testAlterSchema() throws IOException { | |||
|
|
|||
| @Test | |||
| public void testDropSchema() throws IOException { | |||
| stubSchema(20L); | |||
| String name = "schema20"; | |||
| String comment = "comment20"; | |||
| String catalogPath = TEST_ROOT_PATH + "/" + "catalog20"; | |||
There was a problem hiding this comment.
@mchades I was thinking about further extracting the schema id (in this case, 20) into a variable, so updating it automatically adjusts all related name, comment, catalogPath, etc. wdyt?
| stubSchema(testId); | ||
| Schema schema = createSchema(schemaName, comment, catalogPath, null); |
There was a problem hiding this comment.
how about moving the stubSchema call into createSchema method?
| Schema schema = createSchema(testId); | ||
| Fileset fileset = createFileset(testId); | ||
| final NameIdentifier filesetIdent = | ||
| NameIdentifier.of("m1", "c1", schema.name(), fileset.name()); |
There was a problem hiding this comment.
@mchades I've overloaded createSchema and createFileset to take only a testId and stub the schema inside createSchema. The change is only applied to testListFilesetFilesWithNonExistentPath for now. Does this approach seem reasonable, or does the extra overload introduce too much complexity? Also, would switching to an idGenerator instead of manually assigning testId be a better option?
There was a problem hiding this comment.
The primary purpose of this PR is to enable developers to write unit tests more easily by automatically generating IDs, allowing them to focus on the test logic rather than managing IDs manually. You can consider using the idGenerator if it achieves this goal.
| doReturn(new SchemaIds(1L, 1L, testId)) | ||
| .when(spySchemaMetaService) | ||
| .getSchemaIdByMetalakeNameAndCatalogNameAndSchemaName( | ||
| Mockito.anyString(), Mockito.anyString(), Mockito.eq(name)); |
There was a problem hiding this comment.
@mchades thanks for the comment. I've moved the schema-stubbing logic into createSchema so we no longer need to call stubSchema in every test. generateTestId func is added to utilize idGenerator to auto-generate test ids without manually defining a testId. Tests have all passes locally, let me know if you have any other suggestions on the design. thanks!
|
Overall, it's LGTM. This branch cannot be rebased due to conflicts. Could you plz fix it? |
fad251d to
fa5ddba
Compare
fa5ddba to
ae757b9
Compare
|
@mchades rebased done. please take a look. thanks! |
…erations (#7335) ### What changes were proposed in this pull request? Move the schema-stubbing action to its corresponding test. ### Why are the changes needed? In `setUp()` of `TestHadoopCatalogOperations`, we stub schema files in a fixed‐length loop. As we add new tests that require additional schemas to be stubbed (e.g. schema30, schema31, ...) , this manual update of the loop's upper bound is inefficient and throws misleading test failures. Fix: #7280 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? by `TestHadoopCatalogOperations` itself
…alogOperations (apache#7335) ### What changes were proposed in this pull request? Move the schema-stubbing action to its corresponding test. ### Why are the changes needed? In `setUp()` of `TestHadoopCatalogOperations`, we stub schema files in a fixed‐length loop. As we add new tests that require additional schemas to be stubbed (e.g. schema30, schema31, ...) , this manual update of the loop's upper bound is inefficient and throws misleading test failures. Fix: apache#7280 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? by `TestHadoopCatalogOperations` itself
…alogOperations (apache#7335) ### What changes were proposed in this pull request? Move the schema-stubbing action to its corresponding test. ### Why are the changes needed? In `setUp()` of `TestHadoopCatalogOperations`, we stub schema files in a fixed‐length loop. As we add new tests that require additional schemas to be stubbed (e.g. schema30, schema31, ...) , this manual update of the loop's upper bound is inefficient and throws misleading test failures. Fix: apache#7280 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? by `TestHadoopCatalogOperations` itself
…alogOperations (apache#7335) ### What changes were proposed in this pull request? Move the schema-stubbing action to its corresponding test. ### Why are the changes needed? In `setUp()` of `TestHadoopCatalogOperations`, we stub schema files in a fixed‐length loop. As we add new tests that require additional schemas to be stubbed (e.g. schema30, schema31, ...) , this manual update of the loop's upper bound is inefficient and throws misleading test failures. Fix: apache#7280 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? by `TestHadoopCatalogOperations` itself
What changes were proposed in this pull request?
Move the schema-stubbing action to its corresponding test.
Why are the changes needed?
In
setUp()ofTestHadoopCatalogOperations, we stub schema files in a fixed‐length loop. As we add new tests that require additional schemas to be stubbed (e.g. schema30, schema31, ...) , this manual update of the loop's upper bound is inefficient and throws misleading test failures.Fix: #7280
Does this PR introduce any user-facing change?
No
How was this patch tested?
by
TestHadoopCatalogOperationsitself