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

Concurrency fixes for junit-interface #401

Merged
merged 2 commits into from
Aug 20, 2021

Conversation

jenshalm
Copy link
Contributor

This PR attempts to address both issues in #399 (in separate commits).

Regarding 2): I only removed multi-suite support from EventDispatcher and RichLogger, not thread-safety.
Some basic observation suggests that both classes are pinned to a thread, even with asynchronous tests, but I wasn't 100% certain that this is the case in all possible scenarios. Most of the possible simplification is already achieved by dropping multi-suite support, so maybe keeping thread-safety does not hurt.

These are unfixably broken for parallel execution which is the default in sbt.
Addresses point 1) in scalameta#399, see that ticket for more details.
These types map 1:1 to suites/TaskDefs, which means that the complexity
of some of the data structures can be avoided.
Addresses point 2) in scalameta#399, see that ticket for more details.
@jenshalm
Copy link
Contributor Author

The tests did not execute when I originally created the PR, I had to push a random change to actually trigger them. GitHub had issues earlier today, and apparently when checks don't execute during PR creation, they never will.

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution! I apologize the slow review, I've been away on vacation. LGTM 👍

@olafurpg olafurpg merged commit 9cfd8b1 into scalameta:main Aug 20, 2021
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