Skip to content

Update Calcite*Test to use junit5 #16106

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 16 commits into from
Mar 19, 2024
Merged

Update Calcite*Test to use junit5 #16106

merged 16 commits into from
Mar 19, 2024

Conversation

kgyrtkirk
Copy link
Member

@kgyrtkirk kgyrtkirk commented Mar 12, 2024 •
edited
Loading

  • 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.
  • 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
Update Calcite*Test to use junit5
03530aa 
* change the way temp dirs are handled
* add openrewrite workflow to safeguard upgrade
* replace junitparamrunner with standard junit5 parametered tests
* update a few rules to junit5 api
* lots of boring changes
@kgyrtkirk
cleanup QueryLogHook
b95c273
@github-actions github-actions bot added Area - Batch Ingestion Area - Querying Area - Dependencies Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 GHA labels Mar 12, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 12, 2024
View reviewed changes
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@abhishekrb19 abhishekrb19 added Area - Testing and removed Area - Batch Ingestion Area - Querying Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Mar 12, 2024
@gianm
Copy link
Contributor

gianm commented Mar 13, 2024

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

@kgyrtkirk
Copy link
Member Author

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

@gianm
Copy link
Contributor

gianm commented Mar 13, 2024

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?

@github-actions github-actions bot added Area - Batch Ingestion Area - Querying Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Mar 13, 2024
@kgyrtkirk
fix compile error: ARRAYS_DATASOURCE
ffc5062
@kgyrtkirk
update new cases in arraystest
d98db33
@kgyrtkirk
fix test
93950b9
@kgyrtkirk
Merge remote-tracking branch 'apache/master' into calcite-tests-junit5
fdad003
@kgyrtkirk
remove enclosed
805c02e
@kgyrtkirk
empty
02ad253 
+TEST:TDigestSketchSqlAggregatorTest,HllSketchSqlAggregatorTest,DoublesSketchSqlAggregatorTest,ThetaSketchSqlAggregatorTest,ArrayOfDoublesSketchSqlAggregatorTest,BloomFilterSqlAggregatorTest,BloomDimFilterSqlTest,CatalogIngestionTest,CatalogQueryTest,FixedBucketsHistogramQuantileSqlAggregatorTest,QuantileSqlAggregatorTest,MSQArraysTest,MSQDataSketchesTest,MSQExportTest,MSQFaultsTest,MSQInsertTest,MSQLoadedSegmentTests,MSQParseExceptionsTest,MSQReplaceTest,MSQSelectTest,InsertLockPreemptedFaultTest,MSQWarningsTest,SqlMSQStatementResourcePostTest,SqlStatementResourceTest,CalciteSelectJoinQueryMSQTest,CalciteSelectQueryMSQTest,CalciteUnionQueryMSQTest,MSQTestBase,VarianceSqlAggregatorTest,SleepSqlTest,SqlRowTransformerTest,DruidAvaticaHandlerTest,DruidStatementTest,BaseCalciteQueryTest,CalciteArraysQueryTest,CalciteCorrelatedQueryTest,CalciteExplainQueryTest,CalciteExportTest,CalciteIngestionDmlTest,CalciteInsertDmlTest,CalciteJoinQueryTest,CalciteLookupFunctionQueryTest,CalciteMultiValueStringQueryTest,CalciteNestedDataQueryTest,CalciteParameterQueryTest,CalciteQueryTest,CalciteReplaceDmlTest,CalciteScanSignatureTest,CalciteSelectQueryTest,CalciteSimpleQueryTest,CalciteSubqueryTest,CalciteSysQueryTest,CalciteTableAppendTest,CalciteTimeBoundaryQueryTest,CalciteUnionQueryTest,CalciteWindowQueryTest,DecoupledPlanningCalciteJoinQueryTest,DecoupledPlanningCalciteQueryTest,DecoupledPlanningCalciteUnionQueryTest,DrillWindowQueryTest,DruidPlannerResourceAnalyzeTest,IngestTableFunctionTest,QueryTestRunner,SqlTestFrameworkConfig,SqlAggregationModuleTest,ExpressionsTest,GreatestExpressionTest,IPv4AddressMatchExpressionTest,IPv4AddressParseExpressionTest,IPv4AddressStringifyExpressionTest,LeastExpressionTest,TimeFormatOperatorConversionTest,CombineAndSimplifyBoundsTest,FiltrationTest,SqlQueryTest,CalcitePlannerModuleTest,CalcitesTest,DruidCalciteSchemaModuleTest,DruidSchemaNoDataInitTest,InformationSchemaTest,NamedDruidSchemaTest,NamedLookupSchemaTest,NamedSystemSchemaTest,RootSchemaProviderTest,SystemSchemaTest,CalciteTestBase,SqlResourceTest
@kgyrtkirk
use @nested
ab609b4
@kgyrtkirk
Copy link
Member Author

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

gianm
gianm reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

I skimmed the automated changes and tried to focus review on the ones that appeared to have been done by hand. They look OK to me. Just had some questions (see next comment) and one comment about the new surefire config in pom.xml.

pom.xml
@@ -1741,6 +1768,9 @@
<!-- our tests are very verbose, let's keep the volume down -->
<redirectTestOutputToFile>true</redirectTestOutputToFile>
<forkNode implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory"/>
<excludes>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include some comment about why we are doing this. I think it's about removing a default exclusion for static inner classes, but it is not obvious.

@gianm
Copy link
Contributor

gianm commented Mar 18, 2024

@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?

@kgyrtkirk
Copy link
Member Author

kgyrtkirk commented Mar 18, 2024 •
edited
Loading

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

gianm
gianm approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM

@gianm gianm merged commit 0a42342 into apache:master Mar 19, 2024
86 checks passed
@kgyrtkirk kgyrtkirk mentioned this pull request Mar 19, 2024
Merged
@adarshsanjeev adarshsanjeev added this to the 30.0.0 milestone May 6, 2024
@kgyrtkirk kgyrtkirk mentioned this pull request Sep 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianm gianm gianm approved these changes

Assignees
No one assigned
Labels
Area - Batch Ingestion Area - Dependencies Area - Ingestion Area - Lookups Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Area - Querying Area - Testing GHA
Projects
None yet
Milestone
30.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@kgyrtkirk @gianm @abhishekrb19 @adarshsanjeev