Skip to content
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.

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

Fix issue with test event not being generated #297

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

ghale
Copy link
Member

@ghale ghale commented Jul 11, 2024

This is a fix for the issue raised in gradle/gradle#29633.

Build operations for test events are handled in Gradle by registering a TestListenerInternal instance that tracks running tests and generates a build operation when a test starts/stops. We have a hierarchy of operations similar to the following:

Test task
    Test task action 
        Test run 
             Test execution (1..n)
                 Test class (1..n)
                     Test method (1..n)          

When we have an issue with initializing a test during a retry, we end up in a situation where the retry test result processor thinks that another retry is possible, so it does not notify its delegate that the test run is complete. However, the test retry executer sees that there were tests that were marked for retry, but were not retried, and throws an exception. The result of this is that the "test run" build operation is never completed, causing the "dangling build operation" failure.

Reproducing the behavior in the original report is problematic as it requires the develocity plugin which applies its own version of the retry plugin. So the reproducer test only checks that the retry plugin is always calling the started/completed methods for the TestListenerInternal broadcaster for every test descriptor (including the one for the overall test run).

@ghale ghale added the bug Something isn't working label Jul 11, 2024
@ghale ghale self-assigned this Jul 11, 2024
@pshevche pshevche requested a review from a team July 12, 2024 06:38
@pshevche pshevche added the blocked Something prevents from doing this label Jul 12, 2024
@pshevche
Copy link
Member

Blocked until #296 is merged. The test will likely have to go away as the sample won't result in unretried tests anymore. I am not sure how to reproduce it otherwise, though.

@ghale
Copy link
Member Author

ghale commented Jul 13, 2024

One thing that might work would be a JUnit Jupiter parameterized test with a @MethodSource annotation, where the method checks a static counter, or the presence of a file or something and throws an exception on the second execution. I haven't tried this, but it seems like it would cause a similar failure where the test can't be initialized.

Another option would be to turn it into a unit test and recreate the failure that way, but the behavior is so tied in to that of the executer, it might not be as effective.

I don't think it's a big deal if you get rid of the test, but it would be nice to validate that the retry processor properly interacts with its delegate to future-proof the behavior.

@pshevche pshevche force-pushed the gh/issues/dangling-operation branch from 6c1ec1e to 72d35b1 Compare July 15, 2024 13:00
@pshevche pshevche removed the blocked Something prevents from doing this label Jul 15, 2024
@pshevche pshevche force-pushed the gh/issues/dangling-operation branch from 72d35b1 to f981dec Compare July 15, 2024 13:00
@pshevche pshevche force-pushed the gh/issues/dangling-operation branch from d57b636 to 86c0c5d Compare July 15, 2024 13:48
Signed-off-by: Pavlo Shevchenko <[email protected]>
@pshevche pshevche force-pushed the gh/issues/dangling-operation branch from a30f99a to 18fa3e5 Compare July 15, 2024 14:01
@pshevche
Copy link
Member

One thing that might work would be a JUnit Jupiter parameterized test with a @MethodSource annotation, where the method checks a static counter, or the presence of a file or something and throws an exception on the second execution. I haven't tried this, but it seems like it would cause a similar failure where the test can't be initialized.

Another option would be to turn it into a unit test and recreate the failure that way, but the behavior is so tied in to that of the executer, it might not be as effective.

I don't think it's a big deal if you get rid of the test, but it would be nice to validate that the retry processor properly interacts with its delegate to future-proof the behavior.

This, unfortunately, did not work. We can retry failures in the param providers. I experimented a bit with dynamic tests and so on, but with no luck. I'll take another look at it after the release. We need to get it to the customer soon.

@pshevche pshevche force-pushed the gh/issues/dangling-operation branch from 9604c5b to ceba270 Compare July 15, 2024 14:05
@pshevche pshevche merged commit f16a751 into main Jul 15, 2024
6 checks passed
@pshevche pshevche deleted the gh/issues/dangling-operation branch July 15, 2024 14:21
@pshevche pshevche added this to the 1.5.10 milestone Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants