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 test for PriorityConfigurationPlaceholderTaskHelper class #445

Conversation

yashpal2104
Copy link
Contributor

Testing done

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

@yashpal2104 yashpal2104 requested a review from a team as a code owner January 23, 2025 17:40
@github-actions github-actions bot added the tests Automated test addition or improvement label Jan 23, 2025
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.

Need to remove the Javadoc comments. I think that the action being added can be simplified using the same technique that you used in BuildParameterStrategyTest, but I'll need to check further with more review

@github-actions github-actions bot added the dependencies Dependency related change label Jan 27, 2025
@yashpal2104
Copy link
Contributor Author

Hey @MarkEWaite ,
thanks for extending the tests, out of curiosity can I ask what were the tests lacking that I wrote, and also why was GitHub actions components and pipelines were used in extending the tests. Would love to learn more about this.

@MarkEWaite
Copy link
Contributor

thanks for extending the tests, out of curiosity can I ask what were the tests lacking that I wrote, and also why was GitHub actions components and pipelines were used in extending the tests. Would love to learn more about this.

Thanks for asking. I prefer to avoid mock objects as much as I can. They've been more brittle (for me) than tests that use real Jenkins objects.

While I was exploring techniques that might allow the mocking to be replaced with real Jenkins objects, I saw that the ExecutorStepExecution.PlaceholderTask object seems to only be created for Jenkins Pipelines. I thought that I would need a Pipeline job in order to create a Jenkins ExecutorStepExecution.PlaceholderTask object, so I spent the effort to create that Pipeline job. I'd never created a Pipeline job in the priority sorter plugin tests. I learned many things while doing that.

Unfortunately, after all that learning about how to create a Pipeline job inside the priority sorter plugin tests, I still was not able to find a way to create an ExecutorStepExecution.PlaceholderTask object as part of a Jenkins job. I ran out of time that I could give to the priority sorter plugin tests. Rather than abandon the use of a Pipeline job in that test, I kept it as a way to record my learning and broaden the ways that the priority sorter is tested.

My first changes to those tests had used a freestyle project, but when I found that it would need a Pipeline, I stayed with the Pipeline. The first changes helped me learn more about the JobHelper class in the priority sorter plugin tests. It is a very useful test class that can be used in other tests of the priority sorter plugin. It can start one or more jobs and leave them in the queue for use by a test.

I didn't use a GitHub action. The reference to "action" was a reference to a Jenkins action. I was able to remove the reference to Jenkins actions by changing the method used to start the job. That was also part of the learning about the JobHelper class.

@yashpal2104
Copy link
Contributor Author

It is pretty cool to see this, the type of route you took is way beyond mine like creating a pipeline job it didn't occur to me in the slightest and I couldn't have figured it out if not for you. Any knowledge you would be able to impart would be very useful to me.
I think I am still learning to "learn" and seeing this really inspired me. So yes I would love to hear any thoughts about you on this not taking up much of your time obviously just a simple question, How will I be able to do the emulate you lf I wanted to in these situations?
Thank you so much

@MarkEWaite
Copy link
Contributor

It is pretty cool to see this, the type of route you took is way beyond mine like creating a pipeline job it didn't occur to me in the slightest and I couldn't have figured it out if not for you. Any knowledge you would be able to impart would be very useful to me.

I agree that this was something that a first time contributor would not have been able to do.

I think I am still learning to "learn".

Me too. In this case, I was reminded of a concept from a very skilled developer friend. He noted that developers will spend more time (throughout their career) reading code than writing code, so they need to practice reading code even more than they practice writing code.

So yes I would love to hear any thoughts about you on this not taking up much of your time obviously just a simple question, How will I be able to do the emulate you lf I wanted to in these situations?

In this case, the crucial thought was, "someone else has probably already done something like this, where could I find their working examples to learn more?" That's not very helpful for this specific case, but it was the technique that I used.

@MarkEWaite MarkEWaite removed the dependencies Dependency related change label Jan 29, 2025
@MarkEWaite MarkEWaite merged commit 3e05bc5 into jenkinsci:master Jan 29, 2025
18 checks passed
@yashpal2104 yashpal2104 deleted the priority-configuration-placeholder-taskHelper-test branch January 29, 2025 06:28
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