[GitHub] [accumulo] ctubbsii commented on pull request #2553: Refactor testGetByPrefix() test case to improve readability

[GitBox <gi...@apache.org>]

ctubbsii commented on pull request #2553:
URL: https://github.com/apache/accumulo/pull/2553#issuecomment-1062504128


   > > The benefit to these changes is not clear. It's a large change with very little benefit. The motivation seems highly subjective.
   > 
   > The motivation is to simplify this complicated test case and improve the readability for each test scenario. The original case is close to 100 lines of code. After refactoring, each new test case that split from the original case is simple and has a coherent goal. Each case has only about 15 lines of code and describes the test purpose in their test name.
   
   You changed 46 lines of code into 174 lines of code. The original code covered a bunch of trivial cases around a single method. That was more than sufficient, and it was sufficiently readable for its purpose as-is.
   
   Yes, each new test is simpler on its own and covers a more granular test case, but that's not really all that beneficial. We care about test coverage, but we don't have a need for every test case to be that granular. We have thousands of test cases... if we were to break them all into this level of granularity, our code would become indiscriminately large and unwieldy.
   
   > In addition, each test case now will only fail due to one reason, the original test case could fail due to 10 reasons. When using this in the CI\CD check, debug will be more straightforward. (Don't need to run the CI multiple times to locate every defect.)
   
   This test never fails. Optimizing for failure makes no sense here. Debugging these has never been a problem. This test continues to exist to notify us of regressions. If it fails for any one of the 10 reasons, JUnit tells us the exact line number where it failed. That's more than sufficient to track it down in this case. I have literally spent more time reviewing this PR than I have spent debugging this test since it was added.
   
   > This refactor splitting the test case is a time one effort,
   
   It's a huge and unnecessary one-time effort that doesn't benefit the project. But also, it's not a one-time effort. You also tried to do a similar change in PR #2540 that you subsequently closed (perhaps because you deleted your branch and it auto-closed?). These improvements don't seem to be part of any coherent plan to make substantive improvements to Accumulo. They appear to be arbitrary.
   
   >  but it may benefit in the long term for the maintenance.
   
   I'm very confident that no, it won't. At best, it will be neutral. But, at worst, accepting it would encourage more of these kinds of changes that don't really benefit the project, and waste developer time and resources to review.
   
   
   We greatly appreciate that you have an interest in contributing to the project. Even though my words may sound harsh, I genuinely mean that. We always need motivated people, and you certainly seem motivated. But, I don't think we need changes like this. There's simply nobody vying for this kind of change. It's not needed, and it creates more work for the project developers to review, verify, and approve, with no clear substantive benefit to the project.
   
   If you wish to contribute to this project, then instead of contributing changes like these, please consider investigating some of the open issues, or signing up for our mailing list and asking the community what kind of changes they believe would benefit the project, as a way to get started. If you think you have a good idea that isn't an existing issue, feel free to open a discussion thread on the mailing list to get feedback before you start on any big changes.
   
   And please, if you contribute further, please make sure your code actually builds before submitting a pull request. These changes fail at the compile step of our GitHub Actions checks. Those checks can run in your fork before submitting the PR, if you aren't able to run a build on a local development machine. The basic command to make sure everything builds and packages correctly in Maven would be: `mvn package -DskipTests`. If files are reformatted during this step, then make sure to check those in and include those changes in your contribution.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org