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

Implement MUnit integration #119

Merged
merged 8 commits into from May 4, 2020

Conversation

psisoyev
Copy link
Contributor

@psisoyev psisoyev commented Apr 30, 2020

I have implemented MUnit integration.
There is a known issue, though: currently, in MUnit it is not possible to retrieve test status in afterAll/afterEach block so I always pass no error to afterTest method

startedContainers.foreach(_.stop())
}
finally {
startedContainers = None

Choose a reason for hiding this comment

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

Correct me if im wrong, but it looks like you have a potential race condition here. If you add a new container between line 103 and line 106 are executed (and those lines are not atomic), you wont stop the new container. From my understanding all operations on containers should be synchronized

Copy link
Contributor Author

@psisoyev psisoyev May 1, 2020

Choose a reason for hiding this comment

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

Actually I'm not sure it's worth to change state of startedContainers here at all. Probably at this point, it doesn't change anything.

Copy link
Contributor

@LMnet LMnet left a comment

Thanks for your contribution!

Besides integration implementation is would be nice to have documentation about munit module. Could you please add it? Few examples with main concepts would be enough.

Also, it looks like munit integration doesn't support all scalatest integration features. These limitations should be highlighted in the documentation too.

build.sbt Outdated
@@ -98,6 +99,7 @@ lazy val allOld = (project in file("allOld"))
.dependsOn(
core,
scalatest,
munit,
Copy link
Contributor

@LMnet LMnet May 1, 2020

Choose a reason for hiding this comment

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

allOld module is only for backward compatibility. You don't need to add new modules (like munit) to this module.

LMnet
LMnet approved these changes May 1, 2020
Copy link
Contributor

@LMnet LMnet left a comment

Looks good for me!

@dimafeng FYI

@dimafeng
Copy link
Collaborator

dimafeng commented May 2, 2020

@psisoyev thank you for your contribution! I'll take a closer look and merge over the weekend

@psisoyev
Copy link
Contributor Author

psisoyev commented May 2, 2020

I realized that I didn't put MUnit related classes to munit package. I guess that should be done as well. What do you think?

@LMnet
Copy link
Contributor

LMnet commented May 4, 2020

I realized that I didn't put MUnit related classes to munit package. I guess that should be done as well. What do you think?

Yes, unit stuff should be in the separate package.

@dimafeng dimafeng merged commit 306d471 into testcontainers:master May 4, 2020
1 check passed
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.

None yet

4 participants