Add a check to enforce alwaysRun = true on test after-methods #16741

ksobolew · 2023-03-27T10:01:07Z

Description

Turns out that the @AfterX methods do not get executed by default if any of the previous methods failed or were skipped - and that includes the test methods as well. So if any of the tests is skipped, cleanup for the test may also be skipped. The alwaysRun = true annotation enforces that the cleanup method always gets executed.

Additional context and related issues

The failure to run the cleanup method may cause the resource leak detector to trigger and mask the actual cause the test was skipped.

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

ksobolew · 2023-03-27T14:53:26Z

Hm, it works weird. It doesn't fail any tests, just makes it not run any of them at all:

2023-03-27T13:15:24.7677062Z [INFO] -------------------------------------------------------
2023-03-27T13:15:24.7680855Z [INFO]  T E S T S
2023-03-27T13:15:24.7683596Z [INFO] -------------------------------------------------------
2023-03-27T13:15:25.2984005Z [INFO] Running io.trino.plugin.hive.TestHivePlugin
2023-03-27T13:15:25.7540604Z 2023-03-27T07:15:25.499-0600 INFO LogTestDurationListener enabled: true
2023-03-27T13:15:25.7541603Z 2023-03-27T07:15:25.615-0600 INFO ManageTestResources.onStart: running checks
2023-03-27T13:15:25.7775627Z ##[error]Exception in thread "pool-3-thread-1" java.lang.RuntimeException: The @AfterX methods should have the alwaysRun = true attribute to make sure that they'll run even if tests were skipped:
2023-03-27T13:15:25.7786945Z public void io.trino.plugin.hive.TestHivePlugin.deinitializeRubix()
2023-03-27T13:15:25.7788558Z 	at io.trino.testng.services.ReportAfterMethodNotAlwaysRun.checkHasAfterMethodsNotAlwaysRun(ReportAfterMethodNotAlwaysRun.java:66)
2023-03-27T13:15:25.7790085Z 	at io.trino.testng.services.ReportAfterMethodNotAlwaysRun.onBeforeClass(ReportAfterMethodNotAlwaysRun.java:52)
2023-03-27T13:15:25.7790997Z 	at org.testng.internal.TestMethodWorker.invokeBeforeClassMethods(TestMethodWorker.java:167)
2023-03-27T13:15:25.7791529Z 	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
2023-03-27T13:15:25.7792035Z 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
2023-03-27T13:15:25.7792582Z 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
2023-03-27T13:15:25.7793002Z 	at java.base/java.lang.Thread.run(Thread.java:833)
2023-03-27T13:15:25.7793624Z 2023-03-27T07:15:25.700-0600 INFO ManageTestResources.onFinish: running checks
2023-03-27T13:15:25.7794259Z [INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.467 s - in io.trino.plugin.hive.TestHivePlugin

@hashhar do you have any exprience with these listeners?

hashhar · 2023-03-27T18:27:14Z

That's expected and what all our listeners do. See Listeners#reportListenerFailure. Wouldn't we want to know about that instead of just logging a warning which no one would read?

ksobolew · 2023-03-28T08:41:00Z

That's definitely the goal to make it very clear (by failing the tests?). I was following existing listeners, some of which also throw an exception. I was expecting that it would be more visible.

hashhar · 2023-03-28T09:30:58Z

Oh, in terms of how good the error message looks - I agree it's not ideal but that's the best we can do within the limitations of Surefire.

ksobolew · 2023-03-28T09:41:23Z

I would say it's even less than not ideal, because an exception in the listener makes it skip all tests :)

ksobolew · 2023-03-29T07:42:30Z

OK, now it works very loudly ;)

java.lang.RuntimeException: The @AfterX methods should have the alwaysRun = true attribute to make sure that they'll run even if tests were skipped:
public void io.trino.execution.BaseDataDefinitionTaskTest.tearDown()
	at io.trino.testng.services.ReportAfterMethodNotAlwaysRun.checkHasAfterMethodsNotAlwaysRun(ReportAfterMethodNotAlwaysRun.java:77)
	at io.trino.testng.services.ReportAfterMethodNotAlwaysRun.onBeforeClass(ReportAfterMethodNotAlwaysRun.java:55)
        ...


2023-03-28T13:13:36.632-0600	INFO	pool-24-thread-1	stderr	JVM will be terminated

[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  11:21 min
[INFO] Finished at: 2023-03-28T19:13:36Z
[INFO] ------------------------------------------------------------------------
Error:  Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:3.0.0-M5:test (default-test) on project trino-main: There are test failures.
Error:  
Error:  Please refer to /home/runner/work/trino/trino/core/trino-main/target/surefire-reports for the individual test results.
Error:  Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream.
Error:  The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
Error:  Command was ...
Error:  Error occurred in starting fork, check output in log
Error:  Process Exit Code: 1
Error:  Crashed tests:
Error:  io.trino.operator.aggregation.TestApproximateSetGenericDouble
Error:  org.apache.maven.surefire.booter.SurefireBooterForkException: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
Error:  Command was ...
Error:  Error occurred in starting fork, check output in log
Error:  Process Exit Code: 1
Error:  Crashed tests:
Error:  io.trino.operator.aggregation.TestApproximateSetGenericDouble

Thanks @hashhar

This attribute says that this after-method will get executed even if the methods executed previously failed or were skipped. Note that it also applies to skipped test methods, so if the tests were skipped for some reason, the cleanup won't run. This attribute will ensure that the clean-up will run even in this case. Failure to run clean up may cause secondary effects, especially our resource leak detector; failure on this will in turn mask other errors, the ones which caused the tests to be skipped in the first place.

...sting-services/src/test/java/io/trino/testng/services/TestReportAfterMethodNotAlwaysRun.java

hashhar

looks good but I'll defer to @findepi as the listener expert.

findepi · 2023-03-29T15:14:04Z

...o-testing-services/src/main/java/io/trino/testng/services/ReportAfterMethodNotAlwaysRun.java

+    private static final Set<AnnotationPredicate<?>> AFTER_ANNOTATIONS = ImmutableSet.of(
+            new AnnotationPredicate<>(AfterTest.class, not(AfterTest::alwaysRun)),
+            new AnnotationPredicate<>(AfterMethod.class, not(AfterMethod::alwaysRun)),
+            new AnnotationPredicate<>(AfterClass.class, not(AfterClass::alwaysRun)),
+            new AnnotationPredicate<>(AfterSuite.class, not(AfterSuite::alwaysRun)),
+            new AnnotationPredicate<>(AfterGroups.class, not(AfterGroups::alwaysRun)));


the predicate doesn't look for "after annotations". it looks for "after annotations that lack certain attribute"

AFTER_ANNOTATIONS -> VIOLATIONS?

Good idea, changed

See the previous commit for details. This check will enforce that the `alwaysRun = true` is present.

kokosing · 2023-03-31T12:14:05Z

Thank you @ksobolew

cla-bot bot added the cla-signed label Mar 27, 2023

ksobolew force-pushed the kudi/enforce-always-run-on-after-methods branch from 708506f to 2fa9b41 Compare March 27, 2023 11:10

ksobolew changed the title ~~[WIP] Add a check to enforce alwaysRun = true on test after-methods~~ Add a check to enforce alwaysRun = true on test after-methods Mar 27, 2023

ksobolew force-pushed the kudi/enforce-always-run-on-after-methods branch 2 times, most recently from 71d0918 to 85d4938 Compare March 28, 2023 14:38

ksobolew force-pushed the kudi/enforce-always-run-on-after-methods branch from 85d4938 to fa3a618 Compare March 29, 2023 07:44

ksobolew marked this pull request as ready for review March 29, 2023 07:45

ksobolew requested review from kokosing, hashhar, Laonel and findepi March 29, 2023 07:46

kokosing reviewed Mar 29, 2023

View reviewed changes

...sting-services/src/test/java/io/trino/testng/services/TestReportAfterMethodNotAlwaysRun.java Show resolved Hide resolved

github-actions bot added hive Hive connector tests:hive labels Mar 29, 2023

ksobolew force-pushed the kudi/enforce-always-run-on-after-methods branch from fa3a618 to 48c7333 Compare March 29, 2023 09:07

kokosing approved these changes Mar 29, 2023

View reviewed changes

...sting-services/src/test/java/io/trino/testng/services/TestReportAfterMethodNotAlwaysRun.java Outdated Show resolved Hide resolved

ksobolew force-pushed the kudi/enforce-always-run-on-after-methods branch from 48c7333 to 1153f57 Compare March 29, 2023 10:53

hashhar removed their request for review March 29, 2023 11:08

hashhar approved these changes Mar 29, 2023

View reviewed changes

findepi reviewed Mar 29, 2023

View reviewed changes

findepi approved these changes Mar 29, 2023

View reviewed changes

Add a check to enforce alwaysRun = true on test after-methods

bdb3cab

See the previous commit for details. This check will enforce that the `alwaysRun = true` is present.

ksobolew force-pushed the kudi/enforce-always-run-on-after-methods branch from 1153f57 to bdb3cab Compare March 29, 2023 15:19

Laonel approved these changes Mar 30, 2023

View reviewed changes

hashhar merged commit 19c4275 into trinodb:master Mar 30, 2023

ksobolew deleted the kudi/enforce-always-run-on-after-methods branch March 30, 2023 07:57

github-actions bot added this to the 412 milestone Mar 30, 2023

colebow mentioned this pull request Apr 4, 2023

Add Trino 412 release notes #16881

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a check to enforce alwaysRun = true on test after-methods #16741

Add a check to enforce alwaysRun = true on test after-methods #16741

ksobolew commented Mar 27, 2023 •

edited

Loading

ksobolew commented Mar 27, 2023

hashhar commented Mar 27, 2023

ksobolew commented Mar 28, 2023

hashhar commented Mar 28, 2023

ksobolew commented Mar 28, 2023 •

edited

Loading

ksobolew commented Mar 29, 2023

hashhar left a comment

findepi Mar 29, 2023

ksobolew Mar 29, 2023

kokosing commented Mar 31, 2023

Add a check to enforce alwaysRun = true on test after-methods #16741

Add a check to enforce alwaysRun = true on test after-methods #16741

Conversation

ksobolew commented Mar 27, 2023 • edited Loading

Description

Additional context and related issues

Release notes

ksobolew commented Mar 27, 2023

hashhar commented Mar 27, 2023

ksobolew commented Mar 28, 2023

hashhar commented Mar 28, 2023

ksobolew commented Mar 28, 2023 • edited Loading

ksobolew commented Mar 29, 2023

hashhar left a comment

Choose a reason for hiding this comment

findepi Mar 29, 2023

Choose a reason for hiding this comment

ksobolew Mar 29, 2023

Choose a reason for hiding this comment

kokosing commented Mar 31, 2023

ksobolew commented Mar 27, 2023 •

edited

Loading

ksobolew commented Mar 28, 2023 •

edited

Loading