You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/03/08 18:21:02 UTC
[GitHub] [accumulo] Codegass opened a new pull request #2553: Refactor testGetByPrefix() test case to improve readability
Codegass opened a new pull request #2553:
URL: https://github.com/apache/accumulo/pull/2553
## Description
This pull request refactors the test case [testGetByPrefix](https://github.com/apache/accumulo/tree/main/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L206) in test class AccumuloConfigurationTest.
Currently, this test case combines 10 different test scenarios for the getAllPropertiesWithPrefix(). The refactoring breaks this test case down into 10 separate test cases, each of which focuses on one scenario. This will make the test cases smaller and simpler---thus easier to understand and maintain. This should also make debugging easier since each test case is more focused.
Note that this refactor keeps the test logic related to `Property.VFS_CONTEXT_CLASSPATH_PROPERTY`, which will be [deprecated since accumulo 2.1.0.](https://github.com/apache/accumulo//tree/main/core/src/main/java/org/apache/accumulo/core/conf/Property.java#L1177)
## Motivation
- Make test cases smaller, simpler, and easier to understand: The original test case is too long and complicated with combined test scenarios. By extracting the test targets to 10 separate unit test cases focusing on specific test scenarios. Each test case is kept small and simple, and given a meaningful name to show its purpose. And each test case now follows the clear A-arrange, A-action, and A-assert structure, which is easy for anyone to quickly pick up and change in the future.
- Debugging will be much less of a headache. With the original one big test case, if it fails, it is difficult to tell why it fails, due to the cluttered structure. Now, separating into 10 test cases, each test case fails for only one scenario. This makes bugs more difficult to hide and debugging easier.
## Key Changes in this PR
- Replace the [testGetByPrefix](https://github.com/apache/accumulo/tree/main/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L206) test case with 10 unit tests, each unit test case is named based on the test target and its specific action or parameter in the testing:
- [testGetByPrefixWithTableArbitraryPropPrefix](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L226-L240)
- [testGetByPrefixWithTableIteratorScanPrefix](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L243-L257)
- [testGetByPrefixVFSContextClasspathProp](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L260-L270)
- [testGetByPrefixGetOnePrefixNotRegenerateOther](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L273-L291)
- [testGetByPrefixWithOneTimeRegeneratedTableIteratorScanPrefix](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L294-L314)
- [testGetByPrefixWithOneTimeRegeneratedTableArbitraryPropPrefix](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L317-L335)
- [testGetByPrefixWithOneTimeRegeneratedVFSContextClasspathProp](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L338-L355)
- [testGetByPrefixWithTwiceRegeneratedTableIteratorScanPrefix](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L358-L380)
- [testGetByPrefixWithTwiceRegeneratedTableArbitraryPropPrefix](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L383-L403)
- [testGetByPrefixGetOnePrefixNotRegenerateOtherAfterRegenerate](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L406-L424)
- The reused arrangement statements are extracted as [setUpTestConfigForGetByPrefix()](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L206-L217) and
[reSetTestConfigForGetByPrefix()](https://github.com/Codegass/accumulo/tree/refactor-configurationTest/core/src/test/java/org/apache/accumulo/core/conf/AccumuloConfigurationTest.java#L219-L223).
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2553: Refactor testGetByPrefix() test case to improve readability
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #2553:
URL: https://github.com/apache/accumulo/pull/2553#issuecomment-1062505934
I'm going to close this PR based on my above comments. If other PMC members feel these changes are desired, please feel free to re-open.
--
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
[GitHub] [accumulo] ctubbsii closed pull request #2553: Refactor testGetByPrefix() test case to improve readability
Posted by GitBox <gi...@apache.org>.
ctubbsii closed pull request #2553:
URL: https://github.com/apache/accumulo/pull/2553
--
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
[GitHub] [accumulo] Codegass edited a comment on pull request #2553: Refactor testGetByPrefix() test case to improve readability
Posted by GitBox <gi...@apache.org>.
Codegass edited a comment on pull request #2553:
URL: https://github.com/apache/accumulo/pull/2553#issuecomment-1062471775
> The benefit to these changes is not clear. It's a large change with very little benefit. The motivation seems highly subjective.
@ctubbsii Thanks for the reply!
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.
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 refactor splitting the test case is a time one effort, but it may benefit in the long term for the maintenance.
--
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
[GitHub] [accumulo] ctubbsii commented on pull request #2553: Refactor testGetByPrefix() test case to improve readability
Posted by 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
[GitHub] [accumulo] ctubbsii edited a comment on pull request #2553: Refactor testGetByPrefix() test case to improve readability
Posted by GitBox <gi...@apache.org>.
ctubbsii edited a comment 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?) and several other similar changes in branches in your fork (https://github.com/Codegass/accumulo/branches) preparing for other similar pull requests. These improvements don't seem to be part of any coherent plan to make substantive improvements to Accumulo. They appear to be arbitrarily conforming to an arbitrary style of unit test organization that has little to do with Accumulo's needs.
> 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
[GitHub] [accumulo] Codegass commented on pull request #2553: Refactor testGetByPrefix() test case to improve readability
Posted by GitBox <gi...@apache.org>.
Codegass commented on pull request #2553:
URL: https://github.com/apache/accumulo/pull/2553#issuecomment-1062511352
Thanks for the reply, it is very helpful for me to understand the whole picture of the project contribution and the maintainer's concern , I will take time to re-think about the refactor-pr.
Hope I can contribute the proper one in the near future.
--
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
[GitHub] [accumulo] Codegass commented on pull request #2553: Refactor testGetByPrefix() test case to improve readability
Posted by GitBox <gi...@apache.org>.
Codegass commented on pull request #2553:
URL: https://github.com/apache/accumulo/pull/2553#issuecomment-1062471775
> The benefit to these changes is not clear. It's a large change with very little benefit. The motivation seems highly subjective.
@ctubbsii Thanks for the reply!
The motivation is to simplify this complicated test case and improve the readability for each test scenario. 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.
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 refactor splitting the test case is a time one effort, but it may benefit in the long term for the maintenance.
--
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