Add test for PropertyBasedJobInclusionStrategy class #420
Conversation
Added a constructor to initialize description, priority, and jobGroupStrategy fields. This change allows creating JobGroup instances with specific values for these fields.
Do not use wildcard import of org.junit.Assert because that includes the deprecated assertThat method. Use the assertThat method from hamcrest because it is not deprecated and works very well with hamcrest assertions. Name the strategy uniquely for each test by including the test name in the strategy name. The TestName Rule is a very handy way to assure that test data is unique for each test method without needing to worry about the uniqueness directly. Split the Before methods so that they can be named more specifically.
There was a problem hiding this comment.
I've added several assertions to the test class. I'll need to give more thought to the addition of public constructors for test purposes. I think that we might be able to move the test class so that it could use a package protected constructor. However, that needs more time.
|
Alright, what suits the best will work. I thought of making this a draft PR but yeah I thought the changes were valid should we make this a draft PR or is there something else we need to do? |
Don't need a new constructor when setters are already available for all the attributes used in the new constructor.
Reduce the amount of change in the production object by not creating a new method that overrides the method from the superclass. Limit the accessibility of the new method so that it is package protected and not available to classes outside its own package. Remove two duplicated test methods
|
@yashpal2104 I've modified the class in src/main so that it is no longer providing a public method. That felt too invasive for a test related change. I've created a package protected method that is called from the test. Because it is package protected, it is only available to classes that are in the same package. If those changes are acceptable to you, then I believe this is ready to merge. If they aren't acceptable, you are welcome to revert them or submit additional changes to further improve them. |
…tegy
- Added detailed Javadoc comments for `getThisDescriptor` and `getDescriptor` methods.
- Enhanced inline comments to clarify the use of `@SuppressWarnings("unchecked")`.
|
Looks good to me for now. I have added some comments to the changes we made so that we can pinpoint any issues if something goes wrong. If you think this is unnecessary you can remove them. |
Testing done
Add test for PropertyBasedJobInclusionStrategy class
Fixes JIRA issue JENKINS-69757
Submitter checklist