Simplify test task state to make it CC-friendly #25983
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abstratt
force-pushed
the
rchaves/master/make-test-task-listeners-cc-friendly
branch
3 times, most recently
from
d8841a9
to
f1df59d
Compare
abstratt
force-pushed
the
rchaves/master/make-test-task-listeners-cc-friendly
branch
from
f1df59d
to
fe8bf21
Compare
bamboo
reviewed
Aug 1, 2023
subprojects/testing-base/src/main/java/org/gradle/api/tasks/testing/AbstractTestTask.java
Outdated
Show resolved
Hide resolved
| if (broadcaster == null) { | ||
| broadcaster = getListenerManager().createAnonymousBroadcaster(listenerClass); | ||
| for (Object listener: subscribedListeners) { | ||
| if (listenerClass.isInstance(listener)) { |
There was a problem hiding this comment.
💅 A transaction log structure could get rid of these execution time decisions by recording the original configuration time decision.
There was a problem hiding this comment.
I had that at first, but with lack of support for lambdas, quite more code was needed (one inner class for each combination of event vs closure/listener).
subprojects/testing-base/src/main/java/org/gradle/api/tasks/testing/AbstractTestTask.java
Show resolved
Hide resolved
subprojects/testing-base/src/main/java/org/gradle/api/tasks/testing/AbstractTestTask.java
Outdated
Show resolved
Hide resolved
abstratt
force-pushed
the
rchaves/master/make-test-task-listeners-cc-friendly
branch
3 times, most recently
from
70cc4b1
to
7777074
Compare
|
@bamboo I think we covered all the bits we discussed (minus adding guards for listener registration at execution time) |
abstratt
added a commit
that referenced
this pull request
Aug 4, 2023
bamboo
approved these changes
Aug 4, 2023
subprojects/testing-base/src/main/java/org/gradle/api/tasks/testing/AbstractTestTask.java
Outdated
Show resolved
Hide resolved
subprojects/testing-base/src/main/java/org/gradle/api/tasks/testing/AbstractTestTask.java
Outdated
Show resolved
Hide resolved
tresat
approved these changes
Aug 7, 2023
subprojects/testing-base/src/main/java/org/gradle/api/tasks/testing/AbstractTestTask.java
Show resolved
Hide resolved
subprojects/testing-base/src/main/java/org/gradle/api/tasks/testing/AbstractTestTask.java
Show resolved
Hide resolved
| @@ -264,43 +264,52 @@ class TestTaskSpec extends AbstractProjectBuilderSpec { | |||
| 0 * closure._ | |||
| } | |||
|
|
|||
| def "adds listeners and removes after execution"() { | |||
| def "removes listeners after execution"() { | |||
There was a problem hiding this comment.
💭 Do you need to test AbstractTestTask#addTestOutputListener(TestOutputListener) in the same way?
There was a problem hiding this comment.
Yeah, we could and maybe should.
There was a problem hiding this comment.
I couldn't figure how to do that. Couldn't get the event to be observed. Any tips?
There was a problem hiding this comment.
Sorry, I haven't tried anything like this, I just noticed the absence.
abstratt
force-pushed
the
rchaves/master/make-test-task-listeners-cc-friendly
branch
from
c701e3e
to
2183f54
Compare
bot-gradle
added a commit
that referenced
this pull request
Aug 8, 2023
…ndly ListenerBroadcast and friends are complex structures(with lots of lazy init'ing) and pose challenges for correctly storing/restoring their state to/from disk (as required by CC). This changeset simplifies the configuration-time state kept by AbstractTestTask so it only collects references to TestListener and TestOutputListener instances, without instantiating any ListenerBroadcasts. Issue: #25855 Co-authored-by: Rafael Chaves <rchaves@gradle.com>
|
Pre-tested commit build failed. Click here to see all build failures. |
ListenerBroadcast and friends are complex structures(with lots of lazy init'ing) and pose challenges for correctly storing/restoring their state to/from disk (as required by CC). This changeset simplifies the configuration-time state kept by AbstractTestTask so it only collects references to TestListener and TestOutputListener instances, without instantiating any ListenerBroadcasts. Issue: #25855
No synchronization required as listener addition/removal does not need to be thread-safe.
abstratt
force-pushed
the
rchaves/master/make-test-task-listeners-cc-friendly
branch
from
2183f54
to
6dc3fe3
Compare
|
@bot-gradle test and merge |
|
Your PR is queued. See the queue page for details. |
bot-gradle
deleted the
rchaves/master/make-test-task-listeners-cc-friendly
branch
This was referenced Aug 29, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ListenerBroadcast and friends are complex structures(with lots of lazy init'ing) and pose challenges for correctly storing/restoring their state to/from disk (as required by CC).
This changeset simplifies the configuration-time state kept by AbstractTestTask so it only collects references to TestListener and TestOutputListener instances, without instantiating any ListenerBroadcasts.
Issue: #25855