search

junit-team / junit4

A programmer-oriented testing framework for Java.
https://junit.org/junit4
Eclipse Public License 1.0
8.51k stars 3.24k forks source link

Use an indexed for loop to avoid defining an unused variable. #1749

Closed cpovirk closed 1 year ago

cpovirk commented 1 year ago

The background is that we run this code through a compiler that complains about unused variables.

As I see it, it doesn't really make sense for the compiler to complain about an unused variable in this case. But given that it does, this could work around the problem for us.

Since there's no benefit to normal users from this PR, I'd also be OK with just maintaining a patch on our end. It's not hard; I just expect to be asked "Did you try to upstream this?" and so I'd like to be able to answer "yes" :)

In all this, I'm assuming that the looping is intentional. The other possibility would be that we're actually supposed to call addChild only once. But I'm assuming that the Description returned by getDescription() is supposed to include one entry for each Description that will be created during run.

This appears to date back to https://github.com/junit-team/junit4/commit/e07e59eb9d24f6e4fa85dd99f311c1feca6ea983#diff-a36cb3d7d02522c18ce7059634d1cd9adabe56a773024708a53017d07d8900e7R29.

(Arguably, this PR makes the connection between getDescription and run clearer. But if we're concerned about that, maybe a code comment would be called for, and/or maybe run should use an indexed for, too...?)

sormuras commented 1 year ago

I'm fine with proposed change. Perhaps add an inline comment for the reasoning?

cpovirk commented 1 year ago

Thanks. Added. Let me know what you think.

kcooney commented 1 year ago

LGTM.

I looked at the history of this code and it seems like the current behavior is intentional.

kcooney commented 1 year ago

Asking @marcphilipp or @sbrannen to provide a second approval, then I can merge.

cpovirk commented 1 year ago

Sadly no; it's a pretty weird setup that we have going—one of our systems for transpiling to another language, which we don't seem to have a way to support block-level suppressions for.

sbrannen commented 1 year ago

Sadly no; it's a pretty weird setup that we have going

OK. Thanks for the explanation.