Improve test coverage #371
Conversation
|
@MarkEWaite I just noticed that the "equals" test for NextLabelCause has the class set to LabelParameterValue and the report says the coverage of equals is zero. I tried doing it but got some weird "Symmetry" problems. |
Thanks very, very much for detecting my mistake in that file! You are absolutely right that the test of the equals contract in that file is testing the wrong class. I'll need to look further into it. Thanks again for catching that mistake! |
Yup, no worries. Great Work! |
Start the Jenkins controller only once for the entire test so that it runs faster. This is more important on Windows than on Linux, but is not harmful because the tests configure their needed Jenkins configuration at the start of each test. Use nodeName as the isEligible argument rather than label string. Label string did not match a node so would always return false. Remove the label declaration since it is not needed for these tests. Confirm that the Jenkins controller is eligible when it has executors and is not eligible when it does not have executors. Check more details of the descriptors.
|
While reviewing your changes, I expanded the isEligible tests in d19126e . Let me know if any of those changes are an issue for you. |
No it looks great! If anything it has broaden my perspective of how good tests should be written and it also highlighted some nuances that I missed like wrong access modifiers and test cases. |
Assert more values are set by the constructors Use both deprecated constructors in a test and mark the test as deprecated so that the compiler does not warn about the use of deprecated methods. Test agent and controller doList and test a case that doList fails to find the agent label.
The doListNodes checks now cover failure cases
The getNextLabels test needs JenkinsRule and an agent, so those are added to the test. The deprecated constructor tests now check more values that are assigned during object construction.
src/test/java/org/jvnet/jenkins/plugins/nodelabelparameter/LabelParameterDefinitionTest.java
Outdated
Show resolved
Hide resolved
Thank You for the review! |
Unexpected that myValue is not returned by parameterValue.getValue(). Seems to be a bug in the NodeParameterDefinition implementation. NodeParameterDefinition declares a private field 'label' that receives the value instead of it being stored in LabelParameterValue.label but does not override the LabelParameterValue implementation of getValue(). Expanded coverage by using different triggerIfResult values that change the behavior of the constructor. Prefer agents in local variables rather than slaves.
Testing done
Hello @MarkEWaite, added new test cases which improves the test coverage from 80% to 85%.
Thank You.
Submitter checklist