[Maria-developers] MDEV-13550 is test case required?
Hello! Do we have to write test cases for such coding errors? Because this is developer factor: either he does such errors or not. What are conditions of getting PR merged?
Hi, Aleksey! On Oct 13, Aleksey Midenkov wrote:
Hello!
Do we have to write test cases for such coding errors? Because this is developer factor: either he does such errors or not. What are conditions of getting PR merged?
Generally it's preferrable to have a test case for all bugs where it's possible. In your particular PR, it's unlikely that someone will intentionally add memcpy() back. But it's possible that lines around this memcpy would be modified in one of the earlier branches, and during the merge git would automerge by restoring memcpy(). Or that a person resolving the merge conflict would restore the memcpy() without realizing the consequences. So, yes, test case is better than no test case. Sometimes a test case is extremely complex or just impossible to create. So it's generally at the discretion of the reviewer. But in this case you're saying that this will be tested in the versioning.partition test. Perhaps you can simply push it together with versioning.partition test instead of pushing it separately? Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Sergei,
On Sat, Oct 14, 2017 at 9:20 PM, Sergei Golubchik
Hi, Aleksey!
On Oct 13, Aleksey Midenkov wrote:
Hello!
Do we have to write test cases for such coding errors? Because this is developer factor: either he does such errors or not. What are conditions of getting PR merged?
Generally it's preferrable to have a test case for all bugs where it's possible.
In your particular PR, it's unlikely that someone will intentionally add memcpy() back. But it's possible that lines around this memcpy would be modified in one of the earlier branches, and during the merge git would automerge by restoring memcpy(). Or that a person resolving the merge conflict would restore the memcpy() without realizing the consequences.
So, yes, test case is better than no test case.
Sometimes a test case is extremely complex or just impossible to create. So it's generally at the discretion of the reviewer.
But in this case you're saying that this will be tested in the versioning.partition test. Perhaps you can simply push it together with versioning.partition test instead of pushing it separately?
Mmm... I'm afraid that it is reproducible only with versioning code. And I don't know if it will be reproducible when we remove dependence on native partitioning. I would suggest to move towards C++ generally, i.e. make constructors instead of memset(), memcpy(). Making it as coding guide will eventually rectify such cases. Anyway, I guess it's better with PR than without it... ;)
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
-- All the best, Aleksey Midenkov Tempesta Technologies
participants (2)
-
Aleksey Midenkov -
Sergei Golubchik