search

unitycatalog / unitycatalog

Open, Multi-modal Catalog for Data & AI
https://unitycatalog.io/
Apache License 2.0
2.29k stars 363 forks source link

Update Tests to use JUnit5 instead of JUnit4 #202

Closed alexreid-db closed 2 months ago

alexreid-db commented 2 months ago

JUnit5 has been out for a while now and JUnit4 hasn't been updated/supported for years. Most new extensions, etc. only support JUnit5. Don't think we are currently using any intricate junit features, so this upgrade should be pretty straight forward (mostly changing annotations).

filipovskid commented 2 months ago

I can take this up

filipovskid commented 2 months ago

There is a JUnit4 feature used in most, if not all, of the tests that is not supported in JUnit5. The feature in question is class-level parametrization initialized in the BaseServerTest abstract class and subclassed by the majority of other classes. This setup assumes a parameterized configuration of the server which can then be applied across all tests. Additionally, it would be beneficial to know how relevant it is to support different server configurations at this level?

Currently, transitioning these tests without introducing major changes to the testing structure is challenging. One common suggestion has been to use inheritance for all tests and provide different configurations for each test, but this approach isn't sustainable in the long run with the current test structure. Issue #878 is supposed to add class-level parametrization, but its milestone has been moved over the years, so I would not rely on this to be added any time soon.

My next steps are to explore JUnit5 extensions, but I do not see a straightforward solution in that direction either. If anyone has suggestions on how to tackle this issue, please feel free to share.

alexreid-db commented 2 months ago

Thanks @filipovskid for looking into this. From my perspective, I think we should just remove the Parametized stuff from the test code. I think there are other approaches we could use to support running the tests against different running instances of the server if we really wanted to do that (run configurations with different env variables or different properties files, etc.)

Thoughts @creechy @ravivj-db ?

creechy commented 2 months ago

@alexreid-db @filipovskid

... I think we should just remove the Parametized stuff from the test code...

Although it sounds like a useful feature, it doesn't look like it's currently being used in the project. There is just one place where parameters are defined and currently there's only a single parameter value. I'd be in favor of removing the Parameterized stuff for Junit5... and investigating options if/when its needed.

Note, that it's my understanding that although full class level parametrization is not possible with Junit5, you can still parametrize specific test methods, which I see useful to test sets of valid/invalid inputs, for example.

filipovskid commented 2 months ago

While I agree with the discussion that the parametrization should be removed, I'm uncertain why it was added initially. There likely was a rationale for such a structure; otherwise, it wouldn't have been included.

That being said, the current setup of the tests with parameterized server configuration, and the approach of starting a server, seems unconventional. In essence, we are performing integration tests instead of unit tests.

If we can agree to remove the parametrization, I can proceed accordingly and expand on this if needed. Let's see what feedback we receive.

Thank you for the discussion!

alexreid-db commented 2 months ago

@filipovskid Yes, let's just proceed with removing the parameterization