1. If we're going to pick a context in a semi random way, we should document the rules for doing so. I.e. something like the "first context that occurs" in the workspace file.

I've reverted my changes picking a context since I was also not happy with it. I've now gone with finding tests per-(super)context.

2. Did we consider just improving the error message? I don't understand why we should making a decision for the user at all here and tests can certainly be defined in a context dependant way.

This is technically a regression, since something that worked before is no longer working. See #13904. Therefore introducing an error message, especially since in the oxcaml case there is an obvious context to pick, seems like a bad user experience.

For context, oxcaml have been running dune runtest on its own and they have a main context and no default context. There are a few other places in dune that would also break due to making similar assumptions.

At the time of implementing dune runtest for ml tests, it seemed like a sensible choice to pick default, I can see now that is not a good idea.

and tests can certainly be defined in a context dependant way.

Just so we are on the same page, could you elaborate on what you have in mind here?

This is technically a regression, since something that worked before is no longer working.

What was it doing before? I am worried that the behavior before was not reasonable, thus it could make more sense to error out now with a sensible error message instead of silently doing the wrong thing.

@Leonidas-from-XIV Before we started running ml tests, we did not need to know anything about contexts. They were threaded throughout and used by the existing context handling mechanisms.

Since #13064 we now have to classify ml tests, this requires a super context to do so since we use dir_contents to load ml sources.

In 3.22.0 we are picking an arbitrary context in order to carry this out, this fix we will do the classification for each separate context, avoiding the need for a choice.

I don't think we should fixate on this being a regression. If the previous behavior was intentional, I was certainly unaware of it. We haven't offered backwards compatibility for any commands apart from those used for release builds. I don't think we should start now, especially over something so simple. We can fix their CI script.

Instead, let's focus on providing good semantics for this command. If choosing the main context is so obvious, could you spell out the criteria? The previous implementation in this PR was hardly obvious to me. The new implementation is to run the tests in every context, which I suppose is more sensible, but is still incomplete.

What happens when a test is defined in one context but not another? This can happen when a test has enabled_if on it. I guess that in your implementation, that contexts where the test is undefined should be skipped? I don't think it's working this way at the moment.

I argue that running the tests in every context is also bad user experience. The automatic building of everything in every context is a reason why contexts are mostly avoided in the first place. I would instead go for something like this:

1. We can select an individual context with --context like we do for other commands.
2. Absent of a selection, we choose the default context. If there's only one context defined, we can assume it's the default context.
3. To run tests in all contexts, we can either defer to the build command, or introduce something else.

Absent of a selection, we choose the default context. If there's only one context defined, we can assume it's the default context.

Right, this is the part that is missing here actually. The main context from oxcaml is the only context, therefore it is the "obvious choice" that I was mentioning earlier.

My previous idea of picking the first in the list would work in this case too, but would break down when using two non-default contexts. My thinking there was that "at this point, this is on the user", but that's exactly the kind of thinking that caused the issue in the first place.

I think outside of an obvious choice, we should just error. dune runtest already supports --context and building in multiple contexts by passing _build/default _build/other as the args.

Will this fix their CI? No. But at least they can rework it to be dune runtest --context main.

@rgrinberg The fix is to now choose default, if that's not available then the member of the singleton set of contexts and that's not a singleton we error.

This changes the tested behaviour of dune runtest.

From the release perspective, if the decision is that the previous behavior was undefined and that this is not therefor a regression, then I don't think it belongs in the patch release. In particular, if this will not actually fix the build failures reported in #13904, then I don't think it needs to be in 3.22.1. The less that is added there the less risk, and the quicker we can actually fix the genuine regressions.

With the understanding that a software regression is "a type of software bug where a feature that has worked before stops working correctly" and that a fix for such a bug would be restoring the correct behavior of the feature, is this a fix for a regression?

From the release perspective, if the decision is that the previous behavior was undefined and that this is not therefor a regression, then I don't think it belongs in the patch release. In particular, if this will not actually fix the build failures reported in #13904, then I don't think it needs to be in 3.22.1. The less that is added there the less risk, and the quicker we can actually fix the genuine regressions.

Please include this in 3.22.1. It's easier for us to fix dune than it is to communicate workarounds to users. I don't see the risk in including this.

Since it in fact fixes the behavior and doesn't still require a workaround, I think it should indeed be included! My question was at a point in the discussion where that wasn't decided, IIUC. Thanks!