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.

Sign up for GitHub

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

Jump to bottom

Fix NPE for GenericContainer::isRunning (#411) #412

Merged
merged 5 commits into from
Jul 25, 2017
Merged

Fix NPE for GenericContainer::isRunning (#411) #412

merged 5 commits into from
Jul 25, 2017

Conversation

kprokopchik
Copy link
Contributor

No description provided.

bsideup
bsideup requested changes Jul 19, 2017
View reviewed changes
Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

Hey @dhargo,

Thanks for contributing it so quickly!

May I ask you to add a simple test where you just create GenericContainer() object and do isRunning on it?

@bsideup bsideup added this to the 1.5.0 milestone Jul 19, 2017
@kprokopchik
Copy link
Contributor Author

Hi @bsideup,
Sure will do.

bsideup
bsideup reviewed Jul 19, 2017
View reviewed changes
core/src/test/java/org/testcontainers/junit/GenericContainerRuleTest.java Outdated
@@ -145,6 +145,14 @@ public static void setupContent() throws FileNotFoundException {
// }

@Test
public void testIsRunning() {
GenericContainer container = new GenericContainer();
Copy link
Member

Choose a reason for hiding this comment

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

please add try(GenericContainer container = new GenericContainer()) { here (it implements AutoClosable), otherwise the container will remain until JVMs shutdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry - my bad.

bsideup
bsideup approved these changes Jul 19, 2017
View reviewed changes
Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

LGTM

@bsideup
Copy link
Member

bsideup commented Jul 19, 2017

@dhargo Looks good! Only one more thing - please add a record to CHANGELOG.md (sorry for not mentioning that before)

bsideup
bsideup requested changes Jul 24, 2017
View reviewed changes
Copy link
Member

@bsideup bsideup left a comment

Choose a reason for hiding this comment

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

CHANGELOG.md should mention the change

@kprokopchik
Copy link
Contributor Author

Sorry for late CHANGELOG.md update.

@bsideup
Copy link
Member

bsideup commented Jul 25, 2017

@dhargo perfect, thanks! :)

@bsideup bsideup merged commit 94d7641 into testcontainers:master Jul 25, 2017
@kprokopchik kprokopchik deleted the 411-fix-generic-containers-is-running branch July 25, 2017 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsideup bsideup bsideup approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.4.2
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@kprokopchik @bsideup