Algorithm Contribution Unit Test Requirement
patrickyevsukov opened this issue · 14 comments
This issue arose when I was deciding whether or not to merge #458 Please see that pull request for context, and Kenny's comments. I am opening an issue here for discussion of this topic.
Looks like all of the 99 bottles problem algorithms (with one exception) lack tests. @kennyledet, @dalleng, @jcla1, @Yonaba should tests ever be optional? If so, I can update the README to reflect that.
I'd say at the very least an example of the algorithm should be included and unit tests encouraged.
In my humble opinion, tests should be mandatory. Every single tiny bit of algorithm implementation submitted to this repo should include tests.
That's just my point of view.
@patrickyevsukov There's code in the repo that does not comply to that simple requirement. See: https://github.com/kennyledet/Algorithm-Implementations/blob/master/Hamming_Weight/C/grasGendarme/Hamming_Weigth.c
I'm ok with tests being mandatory, but we should plan to add tests to code that has been merged without them.
Yep, @dalleng nailed it. I do agree with @Yonaba that tests should be mandatory, that is definitely the optimal case.
My main concerns were that we'd have to enact this retroactively if we want full consistency, so we'd have to ask contributors who didn't include tests to re-submit. If they never respond, we'd have to either prune their contributions or add them ourselves, which, I honestly wouldn't mind doing as I love this repo, but I probably wouldn't have enough time to do it as efficiently as normally; I'm starting work at Edmodo next Monday.
I have mad love and respect for all of our current contributors as well as the desire for a strong consistency for our repo, and wouldn't want to compromise either, so I propose this solution:
I think the optimal step would be to make it mandatory from this point on, and perhaps as time goes on we can ask contributors who didn't include tests before to re-submit. If they don't respond or they refuse, whoever out of all of us feels most comfortable in the language used could submit them, at any time (just create an issue to remind yourself and fill it whenever most convenient). I'll gladly handle all the Python cases, as it is my strongest lang.
Like @patrickyevsukov noted, this move would allow us to ultimately save more time in the future.
Thoughts?
My main concerns were that we'd have to enact this retroactively if we want full consistency, so we'd have to ask contributors who didn't include tests to re-submit.
Well, not necessarily. We can decide from now on that all contributions should include tests. We can even provide some information on how to write them if we want consistency.A question arises, do we want full consistency for tests ? In that case, we can design a list of tests any implementation of a given algorithm should pass, and implement tests depending on languages accordingly. But my main concern is we should not longer include algorithms/problem sets which doesn't have tests.
As for the the algorithms already included, the maintainers (i.e, us) can fix that. :)
I as just a simple contributor would agree to make the tests mandatory.
Normally this summer I'm going to contribute some extra algorithms in prolog and haskell, I will make sure that these all have the necessary tests.
For some simple algorithms it's more obvious to quickly test in the same file, maybe that is a good middle way? So ask everybody to at least test their code in the same file, if they don't ask them to resubmit?
@Yonaba, precisely. I edited my post a few times with the solution suggestion before I fell asleep, but if you're replying from email you may have gotten the initial incomplete version.
I agree wholeheartedly, and we pretty much came to the same~ conclusion, with the difference in mine being we ask original contributors to re-submit first, as some of us may be very busy in the coming weeks, and then if they don't we handle it, yes.
If we ask contributors first, it more efficiently and fairly distributes the work.
Thank you for your contributions and feedback! I agree with you that we should ask first, most likely through reopening the issues and @ notifying the original contributor.
I agree that including tests in the same file would be acceptable; at this point we should come to an agreement on a naming convention then.
Congrats on the new job, @kennyledet ! That's awesome news!
Thank you all for the feedback on this. It seems we roundly agree that unit tests should be mandatory. I see no reason that previous submissions lacking tests should cause us to hesitate on requiring them moving forward; it would be wonderful if every current submission had test coverage, but we shouldn't make the perfect the enemy of the good.
_Let's resolve to no longer merge submissions that do not provide tests, and kindly ask submitters to include tests if they have not done so._ This should help us avoid issues such as those raised in #462
Thank you for the feedback on the structure of submissions to the repo, @warreee. I'm going to politely disagree that we should deviate from the current convention:
Algorithm_Name/Language_Name/username/filename.extension
Algorithm_Name/Language_Name/username/filename_test.extension
I believe partitioning the test and algorithm logic is beneficial as it allows us to verify algorithms without polluting the actual algorithm implementation with validation code. As an aside, I have not come across submissions in which a user has mixed algorithm and test code in the same file.
Great assessment, sounds good to me man!
Sounds good to me as well!
Thanks all for the feedback on this 😄
Now that this issue is referenced in the README for all to see, I'm going to go ahead and close it.
Hello, everyone. Would not be awesome if we could list resources for writing the tests in the Resources section of the README file!