Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test output interleaving #69

Merged
merged 3 commits into from
Sep 20, 2022
Merged

Conversation

armanbilge
Copy link
Owner

@armanbilge armanbilge commented Sep 20, 2022

I think this fixes the problem noted here:

/* Futures:
* The ordering of output seems to be indeterminant.
* That is, the output from a given test does not always
* appear directly below/after its Suite. It can look
* like the Test belongs in another Suite.
* Naming tests can give a clue that the real work is being done
* and by whom, but the interleaving is, IMO, a defect.
*/

This is annoying but with only a couple test suites I had been ignoring the issue. Fortunately your comment reminded me of scala-js/scala-js-macrotask-executor@2179a6c.

I tested this locally and indeed it seems to have improved the situation.

Copy link
Collaborator

@LeeTibbert LeeTibbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good karma to you for fixing this!

I am glad that you have both wide experience & a good memory.

LGTM

@@ -46,6 +46,7 @@ lazy val tests = crossProject(JVMPlatform, NativePlatform)
.enablePlugins(NoPublishPlugin)
.nativeConfigure(_.dependsOn(core))
.settings(
Test / testOptions += Tests.Argument("+l"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea what the +ell means? I spent some time on the Web looking for it
in both the sbt and munit docs. Seem like kind of a "If this person gets hit by a bus..."
maintenance situation.

Is it something inherited from PR which provided the template for this PR?
That is understandable & far better than the quirks before.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I had to do quite a bit of digging as well. It comes from here:
https://github.com/scalameta/munit/blob/92710a507339d20368d251feadf66e4f9f4e1840/junit-interface/src/main/java/munit/internal/junitinterface/JUnitRunner.java#L64-L102

I believe it is equivalent to setting --logger=sbt. So I will switch to use that and add a comment linking to the above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

It is a delicate balancing act between up-front actually-spent costs spent now versus
envisioned downstream maintenance costs maybe/maybe_not spent by somebody else.

In an open-source project which hopes to attract additional developers, having a
good model/template has value.

In my mind you strike a good balance.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazingly, --logger=sbt does not work ... no idea at this point 😂 so I just added plenty of comments, and kept this current solution.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that --logger=sbt did not work for some reason
Gotta love quirks! Without them we would have nothing to
do but sit on the beach and watch the waves roll in.

You just saved someone, like me, from stepping into a time suck.

@armanbilge armanbilge merged commit ce5b016 into main Sep 20, 2022
@armanbilge armanbilge deleted the pr/fix-test-output-interleaving branch September 20, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants