Global spec/feature run ordering extension with several available run order variants

[kriegaex]

Relates to #1443.

This PR contains support for specification and feature run ordering, including

• a new lifecycle method IGlobalExtension.initSpecs(Collection<SpecInfo>),
• a new interface SpecProcessor with method process(Collection<SpecInfo>),
• a new built-in global run order extension,
• several run orderers (random, alphabetical, annotation-based),
• an @Order(int) annotation for annotation-based run ordering,
• javadoc and Spock manual updates,
• automated tests.

While personally still believing in what I said before...

I decided not to make the run order extension more generic and focus on run order randomisation, because this is the most justifiable use case to be supported by a built-in extension. Something similar to Jupiter's ClassOrderer.OrderAnnotation mode via @Order would still be possible in user-defined custom extensions, but IMO is not a compelling use case to be supported out of the box. I do not see much value in it and never personally wished to use such an option.

... in the meantime I have extended the PR to provide the more generic solution mentioned above.

[codecov]

Codecov Report

Patch coverage: 89.47% and project coverage change: +78.29 🎉

Comparison is base (52e7688) 0.00% compared to head (da3817f) 78.29%.

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #1631       +/-   ##
=============================================
+ Coverage          0   78.29%   +78.29%     
- Complexity        0     4083     +4083     
=============================================
  Files             0      433      +433     
  Lines             0    13162    +13162     
  Branches          0     1692     +1692     
=============================================
+ Hits              0    10305    +10305     
- Misses            0     2224     +2224     
- Partials          0      633      +633     

Impacted Files	Coverage Δ
.../org/spockframework/runtime/model/FeatureInfo.java	92.50% <ø> (ø)
.../extension/builtin/orderer/DefaultSpecOrderer.java	50.00% <50.00%> (ø)
...ork/runtime/SpockEngineDiscoveryPostProcessor.java	91.89% <83.33%> (ø)
...e/extension/builtin/orderer/RandomSpecOrderer.java	84.61% <84.61%> (ø)
...runtime/extension/builtin/orderer/SpecOrderer.java	84.61% <84.61%> (ø)
...nsion/builtin/orderer/AlphabeticalSpecOrderer.java	86.36% <86.36%> (ø)
...va/org/spockframework/runtime/ExtensionRunner.java	98.96% <100.00%> (ø)
...in/java/org/spockframework/runtime/RunContext.java	96.51% <100.00%> (ø)
...kframework/runtime/extension/IGlobalExtension.java	100.00% <100.00%> (ø)
...work/runtime/extension/builtin/OrderExtension.java	100.00% <100.00%> (ø)
... and 3 more

... and 420 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kriegaex]

@leonard84, @Vampire: I think, this is ready for review, now that I have also added automated tests.

[leonard84]

Take a look at https://github.com/spockframework/spock-gr8conf-2019/tree/code/src/main/java/org/gr8conf/spock

If you introduce randomness, then you should also provide a way to recreate the same execution, otherwise debugging issues can become a PITA.
You'll also need to create a new Random with the seed for each spec when randomizing the features, so that you can recreate the same behavior when just executing one test.

[kriegaex]

If you introduce randomness, then you should also provide a way to recreate the same execution, otherwise debugging issues can become a PITA.

That is an interesting thought, which I tend to agree with. I would, however, like to keep this as low-profile as possible. WDYT, would it be OK to keep using the same static single Random instance and just make it public non-final? Then, whenever we want to debug something or ask users to reproduce a situation for us, we could instruct them to inject a seeded random instance in their MCVEs and tell us if they ran the whole class or a single test when reproducing their issues.

My extension is global, not annotation-driven. Exposing the seed via Spock config would mean to add one or two more settings there. For now, I did not create a separate config class / namespace for run randomisation, but simply two separate values for the runner. Adding one or two seeds would pollute the runner namespace too much, I think. If you insist to expose that via Spock config, I would rather add a sub-namespace in the runner section. But I would really like to avoid that.

[leonard84]

I would, however, like to keep this as low-profile as possible.

I've been considering for a while to introduce something like https://junit-pioneer.org/ for Spock extensions. Where they can live and mature at their own pace and potentially be promoted to core if they've proven their usefulness and maturity.

[kriegaex]

I would, however, like to keep this as low-profile as possible.

I've been considering for a while to introduce something like https://junit-pioneer.org/ for Spock extensions. Where they can live and mature at their own pace and potentially be promoted to core if they've proven their usefulness and maturity.

That does not answer my questions. But if you like to get feedback on that idea: It would scare me off. I dislike long-winded, tedious processes with lots of ceremony, and you know that. I spent two spare hours today in order to prepare the PR, then one more for the test. Just like most other contributors, I do not have much more time than that and would lose interest in this contribution quickly. That way, you lose a lot of traction for the project. I saved you time already. Why not collaborate instead? Why don't you help me perfect the extension the way you like it to be, so you feel comfortable merging the PR?

Just commit on top of this PR. We would both save time, have less debates. You get the code you feel comfortable with as a maintainer, while still retaining my commits, giving me some credit and recognition for the contribution. That would be the smart way to motivate and guide contributors.

[leonard84]

That does not answer my questions. But if you like to get feedback on that idea: It would scare me off. I dislike long-winded, tedious processes with lots of ceremony, and you know that.

That is exactly the reason why that project would be worth while. It would have way less hurdles for new extensions as those are as-is code not implying they will be supported indefinitely.
Especially, with extensions there is no real need for every extension to live in spock-core as we can see by the existing third-party extensions.

I spent two spare hours today in order to prepare the PR, then one more for the test. Just like most other contributors, I do not have much more time than that and would lose interest in this contribution quickly. That way, you lose a lot of traction for the project. I saved you time already. Why not collaborate instead? Why don't you help me perfect the extension the way you like it to be, so you feel comfortable merging the PR?

This somehow implies that I have much more time than you at my hands. Sorry, but if you only want to provide proof of concept PRs, which I'll have to finish them myself somehow, then this it not really helpful. If they require additional work I'll put them on my todo list until I have time to work on them and their importance is greater than the other things I need to do.

I'm careful when adding new features as Spock is a mature project, users expect for their tests to continue working across updates. That is why going fast and breaking things is not the correct modus operandi. Adding a bare bones implementation without considering further use-cases will most likely lead to larger changes, leaving us with the decision to add code to keep the old implementation working with the new one, or to break things. I'd like to avoid that by spending more time upfront to do it right the first time.

You even commented multiple times yesterday that you'd like to wait on my feedback before spending time on this PR yet didn't even wait 24h 😔. I could have pointed you to the existing extensions and saved you some time.

[kriegaex]

with extensions there is no real need for every extension to live in spock-core

I do agree with that. But with regard to execution order randomisation, I think it is a different story, and the extension ought to be part of Spock Core, because it is so basic. Comparing Spock to Jupiter, they have one such core extension, and I think we should have one, too (not because Jupiter has it, but because it makes sense that it exists there), albeit just for randomisation, not all the variations they have. Manual ordering really should be outside of Spock core, because it is too exotic. But an opinionated test framework like Spock which also promotes test (order) independency, should have order randomisation out of the box. We already have core extensions which arguably are less helpful or essential.

This somehow implies that I have much more time than you at my hands.

It implies, that for me Spock is a pet project, but a sideshow. For you, it is the main OSS project you work on. My equivalent is AspectJ. You spend more time here and have responsibility as a maintainer. For me the same is true there.

You even commented multiple times yesterday that you'd like to wait on my feedback before spending time on this PR yet didn't even wait 24h 😔.

Well, I simply used the time slice I had at my disposal. Starting something and not finishing it, tends to make it start rotting, because momentum is lost. See Sarek. We lost momentum (and I lost motivation), because it was difficult to get you collaborating back then. So, at some point I focused on other things.

I could have pointed you to the existing extensions and saved you some time.

You already pointed to your Gr8Conf extension in the other thread. But I did not want to copy and paste that one, but create something small, generic and easily maintainable from scratch and the way I felt comfortable with. I still think that an extension should do one thing, one thing only and also do it well. IMO, this PR meets those criteria. The only thing missing is that we agree on a way to inject random seeds.

[kriegaex]

@leonard84, @Vampire: Please note my new commits and the updated PR description. Some additional explanation for why I chose the run orderer approach in combination with the new global extension lifecycle method, giving extension developers access to all spec infos at once:

Using comparators has drawbacks. It is difficult to write a transitively consistent comparator for random ordering. Of course, something like

public int compare(Integer o1, Integer o2) {
  return random.nextInt(3) - 1;
}

would effectively randomise the order, but neither would it be consistent with equals, nor would it make sure that comparisons are transitive, i.e. a < b and b < c would also mean a < c. To achieve that, we either need the whole collection of elements or at least the number of elements, in which case we would have to create a background collection of randomised integers in order to retain a once created order. This would be pretty contrived. Therefore, simply iterating over all specs or features, assigning orders, is preferable IMO.

Besides, a comparator would not generate order numbers either. In theory, we could do completely without SpecInfo.executionOrder. But the same is true for the already existing SpecInfo.executionOrder, and I wanted to be somewhat consistent with that. If we would decide to use a pure comparator-based approach for spec ordering, I would expect to also consistently refactor the existing logic for feature ordering to get rid of the execution order field.

Please try to be open-minded and do not condemn the approach before really inspecting it, weighing pros and cons.

---

Update: (Thinking aloud) Actually, the initial implementation of the random order extension was less intrusive, did not need a lot of infrastructural changes and did not strive to create anything generic where it was not needed. I am strongly in doubt that anyone will use the alphabetical orderer and that providing the manual, annotation-based orderer provides more than marginal (if any) added value to good testing practice The real value still is in the random orderer. I keep thinking "YAGNI" and am wondering, if it was worth the effort to generalise the approach. WDYT? Moreover, now you have what you are often so skeptical about: extra code to maintain and users with questions to support. The initial approach was really more minimal and easier to maintain, focusing on the main user value.

[kriegaex]

FYI, the force-push just now is the result of rebase on master.

[leonard84]

Thank you for the update, the new implementation goes in the right direction, the only thing I'm still not convinced about is the executionOrder on the SpecInfo together with the new initSpecs callback. As now multiple extensions can manipulate that outcome.
Furthermore, we always pay the sorting penalty even if we don't want to change anything.
I would suggest reverting the executionOrderon theSpecInfoand the newinitSpecs, and instead change the SpecOrdererso that it will return a newComparatorwhich can be stateful, this way you can maintain a consistent order even with random ordering, by storing it in aMap<SpecInfo, Integer>`.
Are there any meaningful scenarios which couldn't be solved that way?

I understand if you don't want to implement those changes and I will do them when I get to it.

[kriegaex]

Thanks for the feedback. Before following your advice, I would like to understand your reasoning better, because if I follow advice blindly, the outcome might be suboptimal one more time. So please, allow me a few follow-up questions and remarks:

still not convinced about is the executionOrder on the SpecInfo together with the new initSpecs callback

We are talking about prior art here. I simply followed the pattern I found in FeatureInfo. There, we have the same executionOrder property (field, getter, setter). I simply pulled it up into the parent class SpecElementInfo, so it can be used consistently in both subclasses. I thought you would be pleased that I tried to stick with the current structure instead of re-inventing the wheel. If you ask me to remove that property for specs, it would only be consequent to also remove it from features and solve the ordering problem there in the same way. I think that this is out of scope for this PR.

As now multiple extensions can manipulate that outcome.

So what is the problem with that? Is it bad that multiple extensions can manipulate execution order? That sounds pretty flexible to me. I am envisioning user-defined extensions which can implement a custom ordering, hence the convenient abstract class SpecOrderer implements SpecProcessor, which is easy to extend.

Furthermore, we always pay the sorting penalty even if we don't want to change anything.

Maybe you did not notice the DefaultSpecOrderer with its no-op methods. There is zero penalty.

[kriegaex]

@leonard84, I would appreciate more timely feedback. It has been almost three months now, and I can barely remember the details of this PR. Sorry to say this again, but over the years it has not become any less tedious and disheartening, trying to make any contributions to this project. Please, busy as you might be, try not to make contributors beg for your attention every single time, and try not to make them waste more time explaining themselves than it took to actually make the change. I think, I addressed all your recent concerns in my comments back in May 2023.