-
Notifications
You must be signed in to change notification settings - Fork 348
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
[#2896] improve(IT): Add e2e test case for verifying fileset schema level and kafka topic level #2937
Conversation
...on-test/src/test/java/com/datastrato/gravitino/integration/test/web/ui/CatalogsPageTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is copied from catalogs/catalog-kafka/src/test/resources/run
, I‘m not sure can we reuse it or the current approach is also acceptable?
* @param schemaName The name of the schema. | ||
* @return The default HDFS storage location for the schema. | ||
*/ | ||
private static String defaultBaseLocation(String schemaName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it more reasonable to use setDefaultBaseLocation
for this variable name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's up to the function from HadoopCatalogIT.java
@@ -464,6 +621,143 @@ public void testTreeNodeRefresh() throws InterruptedException { | |||
|
|||
@Test | |||
@Order(20) | |||
public void testViewFilesetCatalog() throws InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testViewFilesetCatalog
and testViewKafkaCatalog
are best placed in the test process above.
I think we should avoid adding other operations in a single function test to make the test process more integrated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
TOPIC_NAME); | ||
Assertions.assertTrue(catalogsPage.verifyTreeNodes(treeNodes)); | ||
// 7. click fileset tree node | ||
String filesetNode = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable name should be kafkaNode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be topicNode, I have changed it correct.
…d kafka topic level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't find a space to use this run
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 140 line of KafkaContainer.java: URL resource = classLoader.getResource("run");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need split a new xxxCatalogPageTest.java
to test kafka or fileset catalogs.
@Order(3) | ||
public void testKafkaTopicDetail() throws InterruptedException { | ||
// 1. click toptic tree node | ||
String topticNode = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo topticNode
and // 1. click toptic tree node
catalogsPage.clickTreeNode(tableNode); | ||
Assertions.assertTrue(catalogsPage.verifyShowTableTitle(TABLE_TABLE_TITLE)); | ||
Assertions.assertTrue(catalogsPage.verifyShowDataItemInList(COLUMN_NAME_2, true)); | ||
Assertions.assertTrue(catalogsPage.verifyTableColumns()); | ||
} | ||
|
||
@Test | ||
@Order(18) | ||
public void testOrtherRelationaCatalogTreeNode() throws InterruptedException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo testOrtherRelationaCatalogTreeNode
, Other
??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…hema level and kafka topic level (apache#2937) …a topic level <!-- 1. Title: [#<issue>] <type>(<scope>): <subject> Examples: - "[apache#123] feat(operator): support xxx" - "[apache#233] fix: check null before access result in xxx" - "[MINOR] refactor: fix typo in variable name" - "[MINOR] docs: fix typo in README" - "[apache#255] test: fix flaky test NameOfTheTest" Reference: https://www.conventionalcommits.org/en/v1.0.0/ 2. If the PR is unfinished, please mark this PR as draft. --> ### What changes were proposed in this pull request? Improvement of fileset and kafka test case ### Why are the changes needed? Fix: apache#2896 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? <img width="340" alt="image" src="https://github.com/datastrato/gravitino/assets/9210625/520204d9-2d5b-4050-b020-ad47275d2469">
…a topic level
What changes were proposed in this pull request?
Improvement of fileset and kafka test case
Why are the changes needed?
Fix: #2896
Does this PR introduce any user-facing change?
N/A
How was this patch tested?