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

[#2883] refactor(ITUtils): move assertion functions from AbstractIT for clarity #2900

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

laiyousin
Copy link
Contributor

@laiyousin laiyousin commented Apr 11, 2024

What changes were proposed in this pull request?

Move the two utility-oriented functions, assertionsTableInfo and assertColumn, from AbstractIT.java to ITUtils.java, and make them public.
Update all references to these functions accordingly.

Why are the changes needed?

Currently, the AbstractIT class in the Gravitino project contains two functions, assertionsTableInfo and assertColumn, which are more utility-oriented rather than integral to the abstract test class itself.

Fix: #2883

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

N/A

@jerryshao
Copy link
Contributor

@yuqi1129 would you please help to review? Thanks.

@jerryshao jerryshao requested review from yuqi1129, mchades and FANNG1 and removed request for mchades April 12, 2024 04:16
@@ -53,5 +65,62 @@ public static void overwriteConfigFile(String configFileName, Properties props)
}
}

public static void assertionsTableInfo(
Copy link
Contributor

@mchades mchades Apr 12, 2024

Choose a reason for hiding this comment

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

Can Hive IT and Iceberg IT also reuse this util method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current content of the function assertionsTableInfo in CatalogHiveIT and CatalogIcebergIT differs from that in ITUtils; therefore, I don't think it can be reused.

@yuqi1129
Copy link
Contributor

Verified and it's okay with me.

Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

LGTM

@jerryshao
Copy link
Contributor

Merging to main, thanks @laiyousin for your contribution.

@jerryshao jerryshao merged commit e946923 into apache:main Apr 15, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Move assertion functions from AbstractIT to ITUtils for clarity.
4 participants