Copilot commented on code in PR #3669:
URL: https://github.com/apache/pekko/pull/3669#discussion_r4360237907
##########
actor-tests/src/test/scala/org/apache/pekko/dispatch/MailboxTaskSubmissionSpec.scala:
##########
@@ -77,51 +77,98 @@ class MailboxTaskSubmissionSpec extends PekkoSpec {
"PekkoForkJoinTask" must {
- "preserve the Runnable hash code across fresh wrappers" in {
- for (hash <- Seq(Int.MinValue, -1, 0, 1, Int.MaxValue)) {
- val runnable = new Runnable {
- final override def run(): Unit = ()
- final override def hashCode(): Int = hash
- }
- val first = new PekkoForkJoinTask(runnable)
- val second = new PekkoForkJoinTask(runnable)
+ "use task identity for wrappers of the same or equal Runnables" in {
+ final case class ValueRunnable(value: Int) extends Runnable {
+ override def run(): Unit = ()
+ }
+ val runnable = ValueRunnable(1)
+ val first = new PekkoForkJoinTask(runnable)
+ val second = new PekkoForkJoinTask(runnable)
+ val third = new PekkoForkJoinTask(ValueRunnable(1))
+
+ (first should not).equal(second)
+ (second should not).equal(third)
+ (first should not).equal(third)
+ Seq(first, second, third).foreach(task => task.hashCode() shouldBe
System.identityHashCode(task))
Review Comment:
The tests hard-require `task.hashCode()` to match
`System.identityHashCode(task)`. This is stricter than necessary to prove “not
delegated to the Runnable” and can become brittle if `ForkJoinTask` (or this
wrapper) ever changes to compute an identity-based hash that isn’t exactly
`System.identityHashCode`. A more future-proof assertion is to capture `val
initial = task.hashCode()` and assert it remains stable (and independent from
changes in `runnable.hashCode()`), without equating it to
`System.identityHashCode`.
##########
actor-tests/src/test/scala/org/apache/pekko/dispatch/MailboxTaskSubmissionSpec.scala:
##########
@@ -77,51 +77,98 @@ class MailboxTaskSubmissionSpec extends PekkoSpec {
"PekkoForkJoinTask" must {
- "preserve the Runnable hash code across fresh wrappers" in {
- for (hash <- Seq(Int.MinValue, -1, 0, 1, Int.MaxValue)) {
- val runnable = new Runnable {
- final override def run(): Unit = ()
- final override def hashCode(): Int = hash
- }
- val first = new PekkoForkJoinTask(runnable)
- val second = new PekkoForkJoinTask(runnable)
+ "use task identity for wrappers of the same or equal Runnables" in {
+ final case class ValueRunnable(value: Int) extends Runnable {
+ override def run(): Unit = ()
+ }
+ val runnable = ValueRunnable(1)
+ val first = new PekkoForkJoinTask(runnable)
+ val second = new PekkoForkJoinTask(runnable)
+ val third = new PekkoForkJoinTask(ValueRunnable(1))
+
+ (first should not).equal(second)
+ (second should not).equal(third)
+ (first should not).equal(third)
+ Seq(first, second, third).foreach(task => task.hashCode() shouldBe
System.identityHashCode(task))
+
+ val submissions = new java.util.HashSet[PekkoForkJoinTask]()
+ submissions.add(first) shouldBe true
+ submissions.add(second) shouldBe true
+ submissions.add(third) shouldBe true
+ submissions.size() shouldBe 3
+ submissions.remove(second) shouldBe true
+ submissions.contains(first) shouldBe true
+ submissions.contains(third) shouldBe true
+ }
- withClue(s"Runnable hash code $hash: ") {
- first.hashCode() shouldBe hash
- second.hashCode() shouldBe hash
- (first should not).equal(second)
- }
+ "keep its hash code stable when the wrapped Runnable changes" in {
+ var hash = -1
+ val runnable = new Runnable {
+ override def run(): Unit = ()
+ override def hashCode(): Int = hash
}
+ val task = new PekkoForkJoinTask(runnable)
+ val identityHash = System.identityHashCode(task)
+ task.hashCode() shouldBe identityHash
+ hash = 1
+ task.hashCode() shouldBe identityHash
Review Comment:
The tests hard-require `task.hashCode()` to match
`System.identityHashCode(task)`. This is stricter than necessary to prove “not
delegated to the Runnable” and can become brittle if `ForkJoinTask` (or this
wrapper) ever changes to compute an identity-based hash that isn’t exactly
`System.identityHashCode`. A more future-proof assertion is to capture `val
initial = task.hashCode()` and assert it remains stable (and independent from
changes in `runnable.hashCode()`), without equating it to
`System.identityHashCode`.
##########
actor-tests/src/test/scala/org/apache/pekko/dispatch/MailboxTaskSubmissionSpec.scala:
##########
@@ -77,51 +77,98 @@ class MailboxTaskSubmissionSpec extends PekkoSpec {
"PekkoForkJoinTask" must {
- "preserve the Runnable hash code across fresh wrappers" in {
- for (hash <- Seq(Int.MinValue, -1, 0, 1, Int.MaxValue)) {
- val runnable = new Runnable {
- final override def run(): Unit = ()
- final override def hashCode(): Int = hash
- }
- val first = new PekkoForkJoinTask(runnable)
- val second = new PekkoForkJoinTask(runnable)
+ "use task identity for wrappers of the same or equal Runnables" in {
+ final case class ValueRunnable(value: Int) extends Runnable {
+ override def run(): Unit = ()
+ }
+ val runnable = ValueRunnable(1)
+ val first = new PekkoForkJoinTask(runnable)
+ val second = new PekkoForkJoinTask(runnable)
+ val third = new PekkoForkJoinTask(ValueRunnable(1))
+
+ (first should not).equal(second)
+ (second should not).equal(third)
+ (first should not).equal(third)
+ Seq(first, second, third).foreach(task => task.hashCode() shouldBe
System.identityHashCode(task))
+
+ val submissions = new java.util.HashSet[PekkoForkJoinTask]()
+ submissions.add(first) shouldBe true
+ submissions.add(second) shouldBe true
+ submissions.add(third) shouldBe true
+ submissions.size() shouldBe 3
+ submissions.remove(second) shouldBe true
+ submissions.contains(first) shouldBe true
+ submissions.contains(third) shouldBe true
+ }
- withClue(s"Runnable hash code $hash: ") {
- first.hashCode() shouldBe hash
- second.hashCode() shouldBe hash
- (first should not).equal(second)
- }
+ "keep its hash code stable when the wrapped Runnable changes" in {
Review Comment:
The test name says “when the wrapped Runnable changes”, but the wrapped
`Runnable` instance does not change—only its `hashCode()` result changes via a
mutable var. Renaming to something like “keep its hash code stable when the
wrapped Runnable hashCode changes” (or similar) would more accurately describe
the behavior being tested.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
