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

Add tests to SpecialValueParameterResolverExtensionTest #349

Conversation

yashpal2104
Copy link
Contributor

@yashpal2104 yashpal2104 commented Oct 25, 2024

I have tried to increase the branch coverage in the Specialvalueparameterresolverextensiontest class form 33% to 50%

Testing done

Automated tests pass

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did

…ionTest.java class in the embeddable status plugins

line coverage is 92% and branch coverage increased to 50%
…ionTest.java class in the embeddable status plugins

line coverage is 92% and branch coverage increased to 50%
…ionTest.java class in the embeddable status plugins

line coverage is 92% and branch coverage increased to 50%
@yashpal2104 yashpal2104 requested a review from a team as a code owner October 25, 2024 10:15
@github-actions github-actions bot added the tests Automated test addition or improvement label Oct 25, 2024
@MarkEWaite MarkEWaite changed the title Added more tests for the class Specialvalueparameterresolverextensiontest Add tests to SpecialValueParameterResolverExtensionTest Oct 25, 2024
Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. The coverage increase seems to be 0%, but the tests are not harmful and they run quickly enough that they may help.

Comment on lines 5 to 6
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.mock;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the addition, but this source file already uses a Mockito.mock() call without needing a static import. Let's keep it consistent within the source file. Likewise, the source file uses Hamcrest assertions rather than assertEquals from JUnit. This is a good place for the static import of nullValue().

Suggested change
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.mock;
import static org.hamcrest.Matchers.nullValue;

@Test
void resolveWithNullParameter() {
String actualParameter = extension.resolve(mockRun, null);
assertThat(actualParameter, is((String) null));
Copy link
Contributor

Choose a reason for hiding this comment

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

Hamcrest has a method that does not require casting null to the desired type.

Suggested change
assertThat(actualParameter, is((String) null));
assertThat(actualParameter, is(nullValue()));

when(job.getLastBuild()).thenReturn(null);

String parameter = "${buildId}${buildNumber}";
assertEquals("${buildId}${buildNumber}", extension.resolve(job, parameter));
Copy link
Contributor

Choose a reason for hiding this comment

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

The assertEquals method provides a less helpful message on failure than assertThat from Hamcrest. Let's follow the pattern of using Hamcrest assertions in this file. Since the parameter is declared with that value, let's assert that is the correct value rather than copying the string.

Suggested change
assertEquals("${buildId}${buildNumber}", extension.resolve(job, parameter));
assertThat(extension.resolve(job, parameter), is(parameter));

Comment on lines 96 to 101

@Test
void testResolveWithNoMatchingParameters() {
String actualParameter = extension.resolve(mockRun, "noMatchingParameters");
assertThat(actualParameter, is("noMatchingParameters"));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem to test anything more than is already tested by unknownParameterShouldNotBeResolved. I think it should be removed as a duplicate of unknownParameterShouldNotBeResolved

Suggested change
@Test
void testResolveWithNoMatchingParameters() {
String actualParameter = extension.resolve(mockRun, "noMatchingParameters");
assertThat(actualParameter, is("noMatchingParameters"));
}

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Thanks!

@MarkEWaite MarkEWaite merged commit f29a920 into jenkinsci:master Oct 25, 2024
18 checks passed
@yashpal2104
Copy link
Contributor Author

Welcome, Hey @MarkEWaite I cant seem to find any answers to my question on how do I go about adding more automated tests that would help increase the coverage for the classes I really want to contribute and am eager to learn things even if they force me to go outside my comfort zone, I am not getting any definitive answers or even approach that would help me point in the right direction. I am getting ignored by all, I prefer a stern way of knowing if my requests are not appropriate and more clear. Do help me I would really enjoy to do hard things even if they are beyond my reach now maybe expanding my time horizon would make things fit more perfectly in the long run

@yashpal2104
Copy link
Contributor Author

Thanks for the pull request. The coverage increase seems to be 0%, but the tests are not harmful and they run quickly enough that they may help.

Sorry I did try to increase coverage but I feel short on it. I am learning about JUnit5 as of now to understand more clearly about testing

@MarkEWaite
Copy link
Contributor

MarkEWaite commented Oct 25, 2024

I am not getting any definitive answers or even approach that would help me point in the right direction. I am getting ignored by all.

I'm sorry to hear that your questions are not getting definitive answers. That may indicate that others are too busy to respond or they may not understand how to respond in a way that will help.

I'm in the very late stages of preparing for a major Jenkins release 30 Oct 2024. Jenkins 2.479.1 will drop support for Java 11, require Java 17 as the minimum Jenkins version, upgrade Spring Security 5 to Spring Security 6, upgrade Java EE 8 to Jakarta EE 9, and upgrade Jetty 10 to Jetty 12. You can read the release checklist if you'd like to see some of the steps that are being taken.

I prefer a stern way of knowing if my requests are not appropriate and more clear.

If your requests are not appropriate, I'm sure that someone will tell you. I've not felt any concern with your requests, I just haven't had time to respond to them.

Do help me I would really enjoy to do hard things even if they are beyond my reach now maybe expanding my time horizon would make things fit more perfectly in the long run

I've found that the best way that I can develop automated tests is to use my integrated development environment (IDE) to create empty test methods, then I set breakpoints in the target code and write test code to reach those breakpoints. I often find that the most effective way to write a new test is to read the code that someone else wrote to test something similar.

When I look at the coverage report on the files tab of the ci.jenkins.io build, I see that only 4 of the Java source files show coverage of less than 80%. Those 4 files are:

  • ParameterResolver
  • JobBadgeAction
  • PublicBuildStatusAction
  • AddEmbeddableBadgeConfigStep

Those seem like the best place to spend your effort. The JobBadgeActionTest includes a JenkinsRule that creates a FreesStyleProject and uses a JenkinsRule. That is a very common testing pattern in Jenkins plugins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Automated test addition or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants