Provide information to Suite about other discovered Suites

[neko-kai]

Hi, a few years ago we’ve made an open source test framework - distage-testkit. We’ve decided to base it on ScalaTest, instead of writing a new test framework from scratch – which would have required us to write IDE integration from scratch too. It has grown popular now, but since the beginning, we’ve had issues[1] connected to ScalaTest’s SBT Framework runner instantiating suite classes lazily in parallel (unlike Scalatest’s native Runner, which instantiates them eagerly).

Why would such a small detail become a problem? Our test framework has unique features - specifically resource sharing without global variables among test suites - which require orchestration of all tests from a single point, instead of them running independently. To do this, instead of each test running independently, on instantiation suites register themselves and their Reporters in a global collection and the first suite which’s run method is called by ScalaTest actually executes the entire ensemble of suites on its own and uses the stored Reporters to report the results of all other suites.[2]

This works well with ScalaTest’s regular Runner - and in IntelliJ which uses it. Because regular Runner instantiates suites first, before running them in parallel, we know that by the time the first run method is called, all suites have registered themselves, so the list of registered tests contains all the tests we have to run.

But this scheme breaks down in SBT, because in ScalaTest’s Framework suite instantiation happens inside of the returned TaskDef, not before – instantiations and run’s happen in parallel with each other. So there’s reasonable no way to know when all the discovered suites have been instantiated or ran, any time run method is called, more suites could still be instantiated and registered later with no way to find which suite is the last one – we lose the ability to know which suites sbt / ScalaTest have discovered and which we should run.
So, when running under SBT we perform discovery on the classpath ourselves, instead of relying on sbt, because by the time a suite is ran there’s no way to recover the information sbt passed to ScalaTest about the discovered suites. Doing discovery again is error-prone - we don’t have all the information sbt has, and e.g. we have to figure out whether suites belong to the current module or not using a heuristic[3][4][5] – this is something sbt already knows and automatically avoids tests that come from dependencies.

At this point, I believe the only way to fix these issues for good is to fix them at ScalaTest’s level - either by adding information about the other discovered Suites to Args that ScalaTest passes to the Suite, or by making Framework behave the same way as Runner does and instantiate all the Suites first before running them in parallel – so that we can rely on side effects during Suite instantiation to find all the running suites.

In this PR I opt for the second option. I may be wrong, but I think it’s a smaller change and it would be easier to accept into ScalaTest codebase.

[cla-bot]

Hi @neko-kai, we require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please access https://www.artima.com/cla/choose-type to sign our Contributor License Agreement. Your effort is highly appreciated. Thank you.

[artimasites]

@cla-bot[bot] check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[cheeseng]

@neko-kai I like it being consistent with Runner, but likely there are cases that the current lazy behavior is preferred. I wonder if we can make it as an option?

[neko-kai]

@cheeseng There may be such a case, e.g. if a user puts heavy initialization, such as testcontainers, into val in the suite constructor, in this case the tests may start faster if construction happens in parallel. But, I believe this case is abnormal - heavy per-suite initialization should happen in beforeAll/afterAll, and this case has always been broken in Runner – and no one ever complained or noticed it seems.

Either way, making this behavior optional somewhat breaks the purpose of this change for distage-testkit's usecase: we want to be able to drop all our custom classloading logic and rely on initialization order to retrieve the available suites. If the user is able to pass an -L parameter to runner to force lazy initialization, then this will silently break distage-testkit – and we wouldn't be able to detect it and abort run because ScalaTest doesn't pass down the command-line arguments to suites, as far as I remember.
I could add the option for laziness and also a field lazyInitialization: Boolean to Args, so that in distage-testkit we can fail all the suites if it's enabled – that'll work. But I think that option may be a bit redundant, I doubt anyone will experience a noticeable enough slowdown to notice the change, find the cause and then enable the -L option – they may be more likely to fix their suites to use beforeAll/afterAll at that point.

[cheeseng]

@neko-kai We could pass that switch through system props or env var, we have one that uses env var here:

https://github.com/scalatest/scalatest/blob/main/jvm/scalactic-macro/src/main/scala/org/scalactic/source/PositionMacro.scala#L31

The main question would be what should be the default? May be it should be eager since it is consistent with Runner, but following SBT's way which encourages laziness, and it is the current behavior so we want to be more careful for changing it, let's see what @bvenners think?

[neko-kai]

@cheeseng Setting the default to lazy in Runner will now additionally break distage-testkit under Intellij / Runner. There would be no real way to overwrite the default laziness at project level in SBT to unbreak it either, because:

1. If set via system prop, system props can't be set in SBT. In Intellij they can only be set globally via custom VM options.
2. If set via env var, env vars can be set via envVars key in SBT, but it only applies if Test / fork := true is also set. Intellij does not inherit envVars key from SBT at all, it has to be set either globally via shell or individual test run configurations per-test.
3. If set via command line option, Intellij's ScalaTest runner doesn't inherit options set in SBT via Test / testOptions.

Per above I don't see a reasonable way to flip the laziness/strictness switch on the project level such that it applies to both Intellij and SBT. I think the only way to set it portably across any way to run ScalaTest would be if it was set via annotation, trait or a global variable – at source level, not at runner level. Which I think may be a bit too much engineering for adding a switch few would want to ever use. I think there are only two (ok, three) ways here:

1. Use strict initialization everywhere.
2. Use lazy initialization everywhere and add the list of all discovered suite classes to Args, so that we can use it in distage-testkit to avoid custom discovery.
3. Use lazy initialization everywhere and add a marker trait for suites that need strict initialization. (I believe this solution is the most hacky of them, but why not)

[cheeseng]

@neko-kai One thing in sbt is that the discovery was done by sbt not scalatest, while Runner does both discovery + execution of tests. In the sbt case, sbt is responsible for discovery of suites to run and how to run them, whether parallel, fork etc., and ScalaTest's Framework's is responsible to run the suite defined by sbt's TaskDef. If my memory serves me right, we wanted to make the instantiation of Suite lazy also in Runner but we couldn't, probably due to we need to call some method(s) on the suite instance during the discovery phase.

[neko-kai]

@cheeseng There are calls to .suiteId and .testNames Suite methods in Runner [1], but I think they can be deferred to when the suite is being started, same as they're deferred in Framework, if need be.

[cheeseng]

@neko-kai Yes there was also styleName before this but is now dropped in main branch for the upcoming version 3.3.0.

Anyway I am exploring your option 2, I wonder if we add DiscoveryStarting and DiscoveryCompleted to SBT support will it help in your case? I think we can add the list of suite names discovered by SBT/Runner in DiscoveryCompleted if that helps?

[neko-kai]

@cheeseng DiscoveryCompleted events are sent to Reporter, but I don't think there's a way to set / override a Reporter from inside the Suite. We receive a Reporter from ScalaTest in Args.reporter field - but we can't send our own reporter back up to ScalaTest so that it can receive those events.

[cheeseng]

@cheeseng DiscoveryCompleted events are sent to Reporter, but I don't think there's a way to set / override a Reporter from inside the Suite. We receive a Reporter from ScalaTest in Args.reporter field - but we can't send our own reporter back up to ScalaTest so that it can receive those events.

@neko-kai I think you can register a custom Reporter to get the event, but that could be less ideal. Anyway after looking around I think we may pass discovered suites to run (either from Runner or SBT) using either Args, Filter, Filter's DynaTags or ConfigMap. I am not sure which is the best, I'll try to discuss with @bvenners .

Does making the discovered suites class names going to fulfill your needs?

[neko-kai]

@cheeseng Yes, fully qualified class names will work, although Class<?> instances would be slightly better. Best would be also getting a lazy handle to the actual suite instance () => Suite as well, to allow to synchronously force evaluate the suites instead of waiting asynchronously for them.

[neko-kai]

@cheeseng Any updates on exploring the option 2? I can implement it in this PR if you don't have the time to deal with it.

[cheeseng]

@neko-kai Sorry for the delay response, for me option 2 is fine is just I am not sure what's best to go with (Args, Filter, Filter's DynaTags or ConfigMap), I am trying to discuss with @bvenners to make the call but he's busy this week, I shall meet him in the coming week or week after that and hopefully we can move on from there. :)

[neko-kai]

@cheeseng Hi, I've replaced my previous changes in this PR with an implementation of option 2, with some caveats:

First, I didn't make instantiations in Runner lazy. Unfortunately it's not possible to make instantiations in Runner lazy because it reports a RunStarting event with the expected test count[1], calling the expectedTestCount method of the Suite requires it to be instantiated. If we report 0 expected tests in RunStarting as we do in SBT framework, we get a slightly degraded experience in Intellij - the test counter doesn't have the expected number of tests ahead of time and becomes populated only as tests are running. To avoid breaking this, I did not add laziness to Runner's instantiations.

Second, I added information about running suites into Args case class. I didn't add them to Filter, DynaTags or ConfigMap because:

• Filter: is chiefly about test filtering, I don't think it's a good idea to add this data there
• DynaTags: is also about test filtering / test selection via tags, information about other suites has nothing to do with test selection
• ConfigMap: Normally it contains only properties passed via command-line by the user. It's also serialized into JSON in RunStarting event. I don't think it's a good idea to put internal state there, where it will be serialized and dumped into user files.

So, I've added this data into Args, with other transient, run-specific data. The downside is that this breaks binary compatibility - but it doesn't seem to be an issue for 3.3.0 series, where Args binary compatibility was already broken due to removal of chosenStyles field.

[bvenners]

@neko-kai How does having the full list of discovered suites actually help you, since sbt would still be running them in parallel in ad-hoc order?

[neko-kai]

@bvenners Only the first distage-testkit suite actually runs the tests – coordinated via ConcurrentHashMap.putIfAbsent – other suites exit without running any tests if the lock was taken before them. However the first suite still needs access to the other suite instances to collect the tests and Reporters from.

[bvenners]

@bvenners Only the first distage-testkit suite actually runs the tests – coordinated via ConcurrentHashMap.putIfAbsent – other suites exit without running any tests if the lock was taken before them. However the first suite still needs access to the other suite instances to collect the tests and Reporters from.

Ok, does the distage-testkit suite that wins take care of finding and instantiating the other suites, and that's what it does with the list of them?

[bvenners]

@neko-kai I'm wondering if instead of using putIfAbsent you could use a concurrent data structure that separates the initial from the rest, but accumulates the rest as well as storing the initial. Then have the initial asynchronously grab the suites in the rest list and executes them as they come in. (You could execute them sequentially or in parallel.) The trouble with this approach would be you might not know when to finish, so you might empty the rest list before it was fully populated. I think you could probably do a heuristic and just wait a second or two after it empties to make sure nothing else gets in there. And when you quit, you set a flag so that if anything actually comes in later it can be detected and cause a suite aborted, or probably better, it starts over with a new initial.

[neko-kai]

@bvenners We already store the rest in a concurrent structure - when the suite is instantiated, it registers itself. This works well with Runner, where instantiations have all finished by the time the winner suite is chosen, it can then immediately proceed to execution. But not in SBT where we don't know when the structure is fully populated.

We already considered the scheme with waiting, as detailed in the first post, and chose to instead do classpath scanning to discover other suites when running on SBT, to avoid unacceptable non-determinism and delays on start. I've experimented with adding delays at the very beginning of distage-testkit and it wasn't an acceptable experience.

But custom discovery may be error-prone and it breaks sbt testOnly - since we don't know which suites sbt/scalatest chose to execute, we're forced to run all of them. In this PR I seek to address this at scalatest level - pass the list of suites chosen by sbt/scalatest down in Args, to make custom discovery unnecessary and unbreak sbt testOnly.

[neko-kai]

@bvenners @cheeseng Any updates or thoughts about this PR? I've implemented option 2 in this PR - add information about other discovered suites to Args, but if you think it's too invasive or otherwise unacceptable, I can implement option 3 - add an annotation for strict initiailzation. That would be mergeable in 3.2 series as well as 3.3 since there would be no changes to Args.

[cheeseng]

@neko-kai Hi sorry for the late response, @bvenners prefer to minimize the changes, fyi I have been trying with Runner's static field also in this branch:

https://github.com/cheeseng/scalatest/tree/feature-runner-discovered-suites

but the runner for scala-js/scala-native isn't quite working yet, we would want to minimize the overhead impact when serializing discovered suites data, perhaps we can write the list to a file instead and read it only when required? That won't work if SBT fork is running in different machine, but I am not sure if that's even possible.

[neko-kai]

@cheeseng It would be somewhat non-trivial to use a file to pass the data in a portable way, because AFAIK sbt doesn't pass the current module's target directory to us, and I don't know a way to reliably figure it out.
Scala-native implements createTempFile, so you could write the list to a file, then pass the filename to worker via serializeTask.
However Scala.js does not implement it, so you would need to write node.js-specific code for that.
And I'm not really sure that the avoided overhead of passing a string (you could avoid parsing it until discoveredSuites() is called) of at most a few kilobytes would be noticeable compared to writing it to a file – and letting sbt do this work avoids littering the system with tempfiles.

Alternatively: we could simply support this for JVM only.

[cheeseng]

Hi @neko-kai , fyi I submitted the following PR to support Runner.discoveredSuites:

#2319

Feel free to see if it is going to help in what you are after.

Cheers.

[neko-kai]

@cheeseng Yes, that's going to help very much, thank you!