Thanks for the refactoring! However, I would rather revert 06f5a94 and f7e435a because IMHO readability outweighs performance optimizations in this case.

@junit-team/junit-committers What do you think?

I don't have a strong opinion about f7e435a. There are some reasons for keeping the old version and some for using the new one.

I agree with @marcphilipp on not merging 06f5a94. The other three commits are improvements that should definitely be merged.

Since Description instances are mutable, f7e435a is an externally visible change. I don't know if it would break anyone, but since the others are luke-warm about it, we should revert it.

I agree that 06f5a94 should be reverted.

Also for 7474220 please don't move describeCause because it makes it harder to see the diffs.

@alb-i986 Do you want to do the suggested changes or shall i take over?

@marcphilipp ya sorry, I'll do as soon as I can. Cheers

About 06f5a94:

Apart from optimization, I think it also helps readability.
As soon as I see a for-each loop, I expect to see the item to be used inside the block. But here it isn't. That's a little bit "unsettling" I think.

About f7e435a:

Since Description instances are mutable, f7e435a is an externally visible change. I don't know if it would break anyone, but since the others are luke-warm about it, we should revert it.

@kcooney
I see what you mean, good point.

That's unfortunate, IMO. Descriptions should be immutable. There's no point in editing a Description after a Runner builds it and returns it, right?
If you think it's worth it, I'll create an issue for that (maybe in junit5?).

Also for 7474220 please don't move describeCause because it makes it harder to see the diffs.

If you mean that it's harder to do the code review, ya, I understand. I should have done the move in a different commit, sorry about that.
Anyway, I was following the Clean code principle "Place a (private) method right after the method using it", to improve readability.
Are you happy with the move in a separate commit?

@alb-i986

That's unfortunate, IMO. Descriptions should be immutable.

I agree. We did look into making Description immutable (with proper builders) but there was a fair amount of work involved in updating the code to use immutable descriptions (particularly in the filtering code) and the changes might break people, so the effort just lost steam.

JUnit5 doesn't use Description internally, so there is no point to creating an issue for this in JUnit5

As for the other changes in this pull, some of them are changing the code from one style to a different style; the benefits of those kinds of changes are somewhat subjective. I can see your point about 06f5a94 from a readability standpoint (though the performance benefits, if any, are small). I don't have a strong feeling about where private methods should be put before or after the method where they are called, but since two people already agreed on the current style (the author and the reviewer) I suggest we should leave describeChild where is was.

Should be good to be squashed now.
Thanks

Thanks! 👍