search

exercism / java-test-runner

GNU Affero General Public License v3.0
9 stars 13 forks source link

add support parameterized tests #82

Closed ssharaev closed 7 months ago

ssharaev commented 7 months ago

resolve #72 Hi! It's my first PR, sorry if I miss something. I chose most verbose display name format for parameterized tests: testIsLeapYear(int, boolean)[1] 2015, false. But we can change it if you think It could be better, available placeholders are here: org.junit.jupiter.params.ParameterizedTest

ssharaev commented 7 months ago

Oh wow this is really great! For some reason I expected this feature to be much harder to implement, glad that it wasn't. 😄

I left a couple of comments I'd like to see addressed, and then I think we can merge it! 🚀

Thank you for your feedback! Yes, I found it surprising too 😄

ErikSchierboom commented 7 months ago

@sanderploegsma @ssharaev I'm not entirely opposed to using parameterized tests, but usually there is one obvious downside: it becomes (much) harder for the student to use TDD and unskip one test at a time. Would that be the case here too?

ssharaev commented 7 months ago

@sanderploegsma @ssharaev I'm not entirely opposed to using parameterized tests, but usually there is one obvious downside: it becomes (much) harder for the student to use TDD and unskip one test at a time. Would that be the case here too?

Yes, you're right, it's hard to unskip one parametrized case, you can only unskip an entire parametrized test set (you can comment argument lines, but it's not really convenient). However parameterized tests support doesn't force us to use only parameterized tests. We can use them only for additional checks for completed solution. What do you think?

ErikSchierboom commented 7 months ago

We can use them only for additional checks for completed solution. What do you think?

That would be fine! As long as they're not the default, I'm fine with it.

sanderploegsma commented 7 months ago

@ErikSchierboom that's a good point, thanks for raising it. I'll definitely keep it in mind!

sanderploegsma commented 7 months ago

Thanks for your contribution @ssharaev 🎉