search

datastrato / gravitino

World's most powerful open data catalog for building a high-performance, geo-distributed and federated metadata lake.
https://datastrato.ai/docs/
Apache License 2.0
621 stars 193 forks source link

[#2198] No need for tests to be public #2199

Open justinmclean opened 4 months ago

justinmclean commented 4 months ago

What changes were proposed in this pull request?

No need for tests to be public.

Why are the changes needed?

For consistency.

Fix: #2198

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Tested locally

qqqttt123 commented 4 months ago

I just take a look at Apache Spark and Apache Iceberg. They all use public for their tests. I have some concern about this. Some base test class will bring some difficulties to write ut. For example, the subclass can't run some test cases of base class if they are put in different packages.

justinmclean commented 4 months ago

We had about 1/2 our tests using it and half not. We should at least be consistent. JUnit documentation suggests not using public - "It is generally recommended to omit the public modifier for test classes, test methods, and lifecycle methods unless there is a technical reason for doing so". (see https://junit.org/junit5/docs/current/user-guide/#writing-tests-classes-and-methods). Also, several IDEs/static code checkers bring this up as an issue to be fixed. I would suggest not using it unless it is needed.

qqqttt123 commented 4 months ago

Think twice. The scene which I mentioned can be solved by modifier protect. If the document gave us advice, I think we can follow it.

yuqi1129 commented 4 months ago

JUnit may not require the modifier to be public, however, for testNG things are totally different, all tests must be explicitly marked as public to make it work well.

justinmclean commented 4 months ago

You'll notice they have been left as is for the Trino tests, and all tests currently pass.

yuqi1129 commented 4 months ago

You'll notice they have been left as is for the Trino tests, and all tests currently pass.

I see.

jerryshao commented 4 months ago

If we don't have a lint check mechanism to check this each time, I think we will always need to fix this. So if there's a very strong benefit to changing this, I'm conservative to this.

justinmclean commented 4 months ago

There's a good possibility other people will check the quality of our software, and it's easy enough to fix. Also see #662

yuqi1129 commented 3 months ago

@justinmclean Can you resolve the conflict and rebase the main branch?

justinmclean commented 3 months ago

Sure, done.

yuqi1129 commented 3 months ago

@jerryshao I'm also neutral about it. Should we merge it?

jerryshao commented 3 months ago

One concern for me is that we need to have a tool that can check this, without a tool we will always need to fix this continuously.

justinmclean commented 3 months ago

If you use sonarlint in your IDE it will check for it, but it shouldn't be a big issue if some JUnit test cases with public accidentally get checked in. However, merging this would set a good example for people who do write unit tests and make it less likely to occur.