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

Update deprecated code to recommended strategy #6078

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cppwfs
Copy link
Contributor

@cppwfs cppwfs commented Nov 15, 2024

This is a continuation of the effort to update deprecated code

Removed AvailablePortMatcherTests because it is a test of a testing component that is no longer needed

Focused on replacing the rule ExpectedException.none() with AssertJ's exception testing facilities

This is a continuation of the effort to update deprecated code

Removed AvailablePortMatcherTests because it is a test of a testing component
that is no longer needed

Focused on replacing the rule ExpectedException.none() with AssertJ's exception testing facilities
Signed-off-by: Glenn Renfro <[email protected]>

cleanup

Signed-off-by: Glenn Renfro <[email protected]>
@@ -51,6 +51,11 @@
<artifactId>spring-boot-starter-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither this, nor awaitility (above) is required as it piggybacks on spring-boot-starter-test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ran a test so in this case awaitility is a required dependency.

@@ -51,6 +51,11 @@
<artifactId>spring-boot-starter-test</artifactId>
<scope>test</scope>
</dependency>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

Neither this, nor awaitility (above) is required as it piggybacks on spring-boot-starter-test.

import java.util.HashMap;
import java.util.Map;
import org.junit.Rule;

import org.junit.Test;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are in here already, do you mind replacing Junit4 test w/ Junit5 test?


@Rule
public ExpectedException exception = ExpectedException.none();
public class AdditionalEnvironmentValidatorTests {

@Test
public void throw_exception_when_additional_environment_variables_contain_docker_variables() {
Copy link
Contributor

Choose a reason for hiding this comment

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

[UNRELATED] Oh my word, who named these tests. Is this python 🤣

exception.expectMessage("cannot exist in your additional environment");
AdditionalEnvironmentValidator.validate(variables);

assertThatIllegalStateException().isThrownBy(() ->AdditionalEnvironmentValidator.validate(variables)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice improvement @cppwfs - thanks.

@BeforeEach
void init() {
MockitoAnnotations.initMocks(this);
autoCloseable = MockitoAnnotations.openMocks(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

[UNRELATED] This was already here I know. The usage of @Mock has turned out to be more hassle than its worth. Like in this case, its a single mock and we have this setup/teardown to deal w/.

Copy link
Contributor

@onobc onobc left a comment

Choose a reason for hiding this comment

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

Nice improvements @cppwfs - thanks you.

Only suggestion is to rid all of these modified tests of the Junit 4 org.junit.Test annotation.

Signed-off-by: Glenn Renfro <[email protected]>
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