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

Replace similar OS EoL tests with @ParameterizedTests #8153

Merged
merged 5 commits into from Jul 5, 2023

Conversation

Chrimle
Copy link
Contributor

@Chrimle Chrimle commented Jun 15, 2023

OperatingSystemEndOfLifeAdminMonitorTest.java contains tests for getting the OS name, and tests for reading documentation based on the OS.

These tests are simple, and only differ in the Strings for the input and expected output from the method(s) tested. Hence, these test cases can be replaced by @ParameterizedTests.

It is certainly possible to reduce the two @MethodSources into one, but would leave one Argument unused in both tests.

@Chrimle Chrimle marked this pull request as draft June 15, 2023 21:40
@NotMyFault
Copy link
Member

Hey @Chrimle,

how did you test the changes proposed? There are several flaws violating Java logic.

Not sure why these changes were not part of the PR...
@Chrimle
Copy link
Contributor Author

Chrimle commented Jul 1, 2023

Hello @NotMyFault !
I'm honestly not sure what had happened, I tried this 'codespaces' for the first time, but it did not push the commit I intended, apologies for that.

I just went back and fixed that locally instead.

@MarkEWaite MarkEWaite added the tests This PR adds/removes/updates test cases label Jul 2, 2023
@MarkEWaite
Copy link
Contributor

No objections from me. The parameterized tests are a nice way of reducing the duplication.

@Chrimle
Copy link
Contributor Author

Chrimle commented Jul 3, 2023

No objections from me. The parameterized tests are a nice way of reducing the duplication.

Happy you like it :)

Question is if the strings should be replaced with constants - for example, the 2 first arguments are common for the 2 ParameterizedTests.

In the example;
Arguments.of("os-release-alma-8", "AlmaLinux.* 8", "AlmaLinux-8.7-Stone-Smilodon"),

The 2 first strings are repeated in the second test:
Arguments.of("os-release-alma-8", "AlmaLinux.* 8", "AlmaLinux 8.7 (Stone Smilodon)"),

This is very minor, and I don't expect anyone to tweak existing test cases. However, in the future, whenever more OS's are added to these test cases - it may be easy to make a mistake?

@Chrimle Chrimle marked this pull request as ready for review July 3, 2023 18:43
@MarkEWaite
Copy link
Contributor

Question is if the strings should be replaced with constants - for example, the 2 first arguments are common for the 2 ParameterizedTests.

I don't think it will help test readability to replace the strings with constants.

The first argument is a file name from the resources. Duplication of the resource filename in the source code is not a concern for me because I think it keeps the test more readable. I have tests in a plugin that iterate over resource filenames so that the resource names are not in the source code. I find it more difficult to diagnose failures in those tests because the resource being read is not immediately apparent in the source.

The second argument is the matching pattern. That matching pattern is duplicated from inside the JSON file so that the test only asserts on the documentation url. The test could be rewritten to use the matching pattern from the JSON so that the duplicate of the matching pattern is removed from the test, but then the test would rely on the JSON file contents more and might fail to detect inconsistencies.

@Chrimle Chrimle changed the title Replace similar OS EoL tests with ParameterizedTests Replace similar OS EoL tests with @ParameterizedTests Jul 3, 2023
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Thanks a lot!


/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 4, 2023
@NotMyFault NotMyFault merged commit 2583c3c into jenkinsci:master Jul 5, 2023
16 checks passed
@welcome
Copy link

welcome bot commented Jul 5, 2023

Congratulations on getting your very first Jenkins core pull request merged 🎉🥳

This is a fantastic achievement, and we're thrilled to have you as part of our community! Thank you for your valuable input, and we look forward to seeing more of your contributions in the future!

We would like to invite you to join the community chats and forums to meet other Jenkins contributors 😊
Don't forget to check out the participation page to learn more about how to contribute to Jenkins.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback tests This PR adds/removes/updates test cases
Projects
None yet
3 participants