druid icon indicating copy to clipboard operation
druid copied to clipboard
verified publisher icon apache

Update Calcite*Test to use junit5

Open kgyrtkirk opened this issue 1 year ago • 4 comments

  • change the way temp dirs are handled
  • add openrewrite workflow to safeguard upgrade; possibly make future changes easier
  • replace junitparamrunner with standard junit5 parametered tests
  • make QueryLogHook less complicated
  • update a few rules to junit5 api
  • lots of boring changes @Before / @BeforeEach and alike
  • remove @RunWith(Nested.class) and deconfigure the default surefire exclusion for nested static classes

Fixes #XXXX.

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Key changed/added classes in this PR
  • MyFoo
  • OurBar
  • TheirBaz

This PR has:

  • [ ] been self-reviewed.
    • [ ] using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
  • [ ] added documentation for new or modified features or behaviors.
  • [ ] a release note entry in the PR description.
  • [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • [ ] added or updated version, license, or notice information in licenses.yaml
  • [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • [ ] added integration tests.
  • [ ] been tested in a test Druid cluster.

kgyrtkirk avatar Mar 12 '24 01:03 kgyrtkirk

What's the openrewrite stuff for? It looks like that is also currently associated with a failing static check.

gianm avatar Mar 13 '24 15:03 gianm

Right now openrewrite is configured to upgrade the Calcite*Test tests to junit5; if we decide to move on upgrading more - we can first change its configuration - let it do the bulk of it.

Having it enable and run on every build will make sure deprecated/etc stuff is not being pulled back in into these files.

The failing check is there because I've configured an extra rule which should be undone....I'll remove it when I'll update the branch next time

kgyrtkirk avatar Mar 13 '24 16:03 kgyrtkirk

Ah, I see the rewrite.yml with the config. Seems neat. I see that the static failure is about too many UnnecessaryThrows, maybe we should remove that rule?

gianm avatar Mar 13 '24 16:03 gianm

there is something with the CalciteSelectJoinQueryMSQTest ; it did run locally for me - but not by the CI. let's see if @Nested helps

kgyrtkirk avatar Mar 14 '24 15:03 kgyrtkirk

@kgyrtkirk,

I see that the static failure is about too many UnnecessaryThrows, maybe we should remove that rule?

How was this resolved? I don't see it failing anymore.

there is something with the CalciteSelectJoinQueryMSQTest ; it did run locally for me - but not by the CI.

Did it help?

gianm avatar Mar 18 '24 15:03 gianm

UnnecessaryThrows How was this resolved? I don't see it failing anymore.

I've removed the global enablement of UnnecessaryThrows ; we can do that later - I was over-zealous adding it right next to pushing the branch - it did not fail on the few modules I was checking :)

there is something with the CalciteSelectJoinQueryMSQTest ; it did run locally for me - but not by the CI. Did it help?

my investigation have lead to uncover that Surefire has that odd exclusion setting - removing all @RunWith(Nested.class) and disabling the exclusion should be much better

there is also one line about this in the junit5 handbook here

kgyrtkirk avatar Mar 18 '24 16:03 kgyrtkirk