improve code quality of bridgedescs module
The code quality of bridgedescs module should be improved using the reports generated by Findbugs and PMD.
Intended results:
- increase test coverage of
org.torproject.collector.bridgedescsto something reasonably close to 100% - add ant tasks for both
- define the set of rules that will be used for all java projects
- improve code quality of the bridgedescs module based on the reports
Activity
changed milestone to %CollecTor 1.2.0
Please find the attached test class which contains a framework for writing tests along with a few tests to start with. Note that it doesn't require any changes to existing code. If possible, we should write tests for the current code before refactoring any of it. Let me know what you think, I'm happy to make changes and create a branch to get this test class merged. And let me know if this is the wrong ticket, and I'll move this elsewhere.
Trac:
Status: new to needs_review
Very good point! I changed the ticket description to include the new tests as additional result.
I ran the
coveragetask, it's already up to 70% line and 56% branch coverage in the bridgedescs module.I'll take a closer look at the code next.
Just from skimming through: could you create separate helper classes for the descriptor builders? And, it should be avoided that a
recentfolder is left after the test. One way to prevent that is to change the Configuration as in MainTest.changeFilePathsAndSetActivation.Trac:
Description: The code quality of bridgedescs module should be improved using the reports generated by Findbugs and PMD.Intended results:
- add ant tasks for both
- define the set of rules that will be used for all java projects
- improve code quality of the bridgedescs module based on the reports
to
The code quality of bridgedescs module should be improved using the reports generated by Findbugs and PMD.
Intended results:
- increase test coverage of
org.torproject.collector.bridgedescsto something reasonably close to 100% - add ant tasks for both
- define the set of rules that will be used for all java projects
- improve code quality of the bridgedescs module based on the reports
-
Trac:
bridgedescs-fb.txt -
Some remarks about the
Configuration:-
using the enum name instead of the string name of a property makes sure there are no nonexistant properties in tests and guards against typos, i.e. `Key.BridgedescsActivated.name()' instead of the string "BridgedescsActivated". When feasible, of course. The next suggestion doen't use the enum names,yet.
-
ConfigurationexendsProperties, for example inSchedulerTest: {{{ private static final String runConfigProperties = "TorperfActivated=true\nTorperfPeriodMinutes=1\nTorperfOffsetMinutes=0\n"- "RelaydescsActivated=true\nRelaydescsPeriodMinutes=1"
- "\nRelaydescsOffsetMinutes=0\n"
- "ExitlistsActivated=true\nExitlistsPeriodMinutes=1\n"
- "ExitlistsOffsetMinutes=0\n"
- "UpdateindexActivated=true\nUpdateindexPeriodMinutes=1\n"
- "UpdateindexOffsetMinutes=0\n"
- "BridgedescsActivated=true\nBridgedescsPeriodMinutes=1\n"
- "BridgedescsOffsetMinutes=0\n";
@Test() public void testSimpleSchedule() throws Exception { ... Configuration conf = new Configuration(); conf.load(new ByteArrayInputStream(runConfigProperties.getBytes())); ... }}}
-
-
-
Trac:
cpd-collector.txt -
Interesting reports!
Regarding the tests, I picked up some of the suggestions you mentioned, but probably not all of them, and pushed branch task-19755 to my public repository.
Should we parallelize our efforts here? For example, you could start fixing the reported bugs and refactor some code (while making sure you don't change the public interface that would break my tests) and I could write more tests (without touching code in
src/main/java/that you're working on).If that's too complicated, either of us could also pause while the other moves forward. Up to you!
-
-
-
I attached a patch simplifying the configuration setting.
It would be good to create methods for the various list manipulations instead of the repeated loops over the descriptor line lists (i.e. more readable and shorter class file in total).
And, some helper classes instead of private inner classes TarballBuilder and the different *DescriptorBuilders would make the test class a little shorter.
I'm sort of wondering if the supply of the tests descriptor tars should be more direct? But that's a bigger change in this huge test class.
-
Replying to iwakeh:
I attached a patch simplifying the configuration setting.
Oh, that looks indeed simpler. Thanks, will apply that.
It would be good to create methods for the various list manipulations instead of the repeated loops over the descriptor line lists (i.e. more readable and shorter class file in total).
Agreed. That's what I meant by providing even more non-test methods and helper classes that will in turn simplify the actual test methods. Will simplify things there.
And, some helper classes instead of private inner classes TarballBuilder and the different *DescriptorBuilders would make the test class a little shorter.
Not so sure about this one. The advantage of making them inner classes is that they can access the outer classes' attributes. Making them separate classes would lead to even more code overall. But let's see how things evolve.
I'm sort of wondering if the supply of the tests descriptor tars should be more direct? But that's a bigger change in this huge test class.
Can you be more specific what change you have in mind there?
Thanks!
-
And, some helper classes instead of private inner classes TarballBuilder and the different *DescriptorBuilders would make the test class a little shorter.
Not so sure about this one. The advantage of making them inner classes is that they can access the outer classes' attributes. Making them separate classes would lead to even more code overall. But let's see how things evolve.
The only attribute used seems bridgeDirectoriesDir, I think.
-
Okay, I incorporated more feedback from this ticket, though I didn't fix all bugs yet. I also didn't start going through the findings from Findbugs et al. I might not get back to this before Friday, so I figured I could push progress for now. Feel free to review now or wait for more progress in the next couple of days.
-
I looked closer at the "recent" path commit, b/c I also provide a commit for that in the branch for #19720 (moved). We arrived at different solutions: You only log the ConfigurationException and I let it escalate. My reasoning is that, if a Path cannot be constructed it means there is an invalid entry in collector.properties. Thus, the module run should fail (this will be logged).
Maybe, a third solution that sets a recent path as attribute will be better?
-
Replying to iwakeh:
I looked closer at the "recent" path commit, b/c I also provide a commit for that in the branch for #19720 (moved). We arrived at different solutions: You only log the ConfigurationException and I let it escalate. My reasoning is that, if a Path cannot be constructed it means there is an invalid entry in collector.properties. Thus, the module run should fail (this will be logged).
Ah, makes sense, happy to use your patch and discard mine.
Maybe, a third solution that sets a recent path as attribute will be better?
What would be the goal there? I agree that the module shouldn't run if the configuration is invalid.
-
Replying to karsten:
Replying to iwakeh:
I looked closer at the "recent" path commit, b/c I also provide a commit for that in the branch for #19720 (moved). We arrived at different solutions: You only log the ConfigurationException and I let it escalate. My reasoning is that, if a Path cannot be constructed it means there is an invalid entry in collector.properties. Thus, the module run should fail (this will be logged).
Ah, makes sense, happy to use your patch and discard mine.
Fine.
Maybe, a third solution that sets a recent path as attribute will be better?
What would be the goal there? I agree that the module shouldn't run if the configuration is invalid.
I noticed that
config.getPath(Key.RecentPath)is called four times inSanitizedBridgesWriter, but actually that call isn't overly expensive. So no need to change more. Using my #19720 (moved) commit is ok. -
add tests also for new code from #20037 (moved).
-
Also implement changes suggested in #19317 (moved):comment:6 (#19317 (moved):comment:7) and #19317 (moved):comment:8.
-
Alright, I cleaned up my earlier task-19755 branch by rebasing it to master and rewriting Git history to make more sense. Please review my task-19755-2 branch. (Note that I did not put in the binary literals suggested on #19317 (moved), because that confuses Cobertura.)
-
Thanks for all these tests!
Now we have a basis for the refactoring of this module.
It seemed shorter to write the code for the suggestions I had. Please, see the seven commits in my branch based on yours.
Cobertura barks on binary literals, but doesn't fail, so I would prefer to use them.
I tried to make the commits independent of each other.
-
Great, thanks for the review and for the additional changes. I made some whitespace fixes to your commit d37e400, removed some checkstyle warnings in other test sources, and added changelog entries. All pushed to master, let me know if something needs more fixing. Closing. Thanks again!
Trac:
Status: needs_review to closed
Resolution: N/A to fixed mentioned in issue #19778 (moved)
mentioned in issue #20037 (moved)
mentioned in issue #20044 (moved)
mentioned in issue #20080 (moved)