Opened 3 years ago

Closed 3 years ago

#19755 closed enhancement (fixed)

improve code quality of bridgedescs module

Reported by: iwakeh Owned by: iwakeh
Priority: Medium Milestone: CollecTor 1.2.0
Component: Metrics/CollecTor Version:
Severity: Normal Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by iwakeh)

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.bridgedescs to 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

Child Tickets

Attachments (6)

SanitizedBridgesWriterTest.java (25.0 KB) - added by karsten 3 years ago.
bridgedescs-fb.txt (4.5 KB) - added by iwakeh 3 years ago.
vanilla fb complaints for bridgedescs module
pmd-collector-bridgedescs.txt (7.8 KB) - added by iwakeh 3 years ago.
pmd for bridgedescs
cpd-collector.txt (3.9 KB) - added by iwakeh 3 years ago.
cpd for bridgedescs
pmd-report-bridgedescs.txt (18.1 KB) - added by iwakeh 3 years ago.
more rulesets used for pmd
0001-Simplify-Configuration.patch (11.0 KB) - added by iwakeh 3 years ago.

Download all attachments as: .zip

Change History (27)

Changed 3 years ago by karsten

comment:1 Changed 3 years ago by karsten

Status: newneeds_review

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.

comment:2 Changed 3 years ago by iwakeh

Description: modified (diff)

Very good point!
I changed the ticket description to include the new tests as additional result.

I ran the coverage task, 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 recent folder is left after the test. One way to prevent that is to change the Configuration as in MainTest.changeFilePathsAndSetActivation.

Changed 3 years ago by iwakeh

Attachment: bridgedescs-fb.txt added

vanilla fb complaints for bridgedescs module

comment:3 Changed 3 years ago by iwakeh

Attached the out-of-the-box vanilla findbugs findings for bridgedescs module.

comment:4 Changed 3 years ago by iwakeh

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.
  • Configuration exends Properties, for example in SchedulerTest:
     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()));
       ...
    
Last edited 3 years ago by iwakeh (previous) (diff)

Changed 3 years ago by iwakeh

pmd for bridgedescs

Changed 3 years ago by iwakeh

Attachment: cpd-collector.txt added

cpd for bridgedescs

comment:5 Changed 3 years ago by iwakeh

Attached analyses for code pattern duplication and pmd result for rulesets java-unusedcode, java-basic, java-design in bridgedescs module.

comment:6 Changed 3 years ago by karsten

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!

comment:7 Changed 3 years ago by iwakeh

Go ahead and add the tests, that would be great.
The refactoring should wait anyway until we have max coverage for this module.

I have other topics on my list for today, so I'm not in your way here.

Changed 3 years ago by iwakeh

Attachment: pmd-report-bridgedescs.txt added

more rulesets used for pmd

comment:8 Changed 3 years ago by karsten

Pushed more tests to the same branch to increase line coverage to 86%. I think I can't go much above that, and I expect a few untested lines to go away in the refactoring process. I'd say we're good to make changes now. And look at all the TODOs I added to the test file for bugs to be fixed...

Changed 3 years ago by iwakeh

comment:9 Changed 3 years ago by iwakeh

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.

comment:10 in reply to:  9 ; Changed 3 years ago by karsten

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!

comment:11 in reply to:  10 Changed 3 years ago by iwakeh

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.

comment:12 Changed 3 years ago by karsten

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.

comment:13 Changed 3 years ago by iwakeh

I looked closer at the "recent" path commit, b/c I also provide a commit for that in the branch for #19720.
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?

comment:14 in reply to:  13 ; Changed 3 years ago by 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.
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.

comment:15 in reply to:  14 Changed 3 years ago by iwakeh

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.
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 in SanitizedBridgesWriter, but actually that call isn't overly expensive. So no need to change more. Using my #19720 commit is ok.

comment:16 Changed 3 years ago by iwakeh

add tests also for new code from #20037.

comment:17 Changed 3 years ago by iwakeh

Milestone: CollecTor 1.1.0CollecTor 1.2.0

comment:18 Changed 3 years ago by iwakeh

Also implement changes suggested in #19317:comment:6 (#19317:comment:7) and #19317:comment:8.

comment:19 Changed 3 years ago by karsten

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, because that confuses Cobertura.)

comment:20 Changed 3 years ago by iwakeh

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.

comment:21 Changed 3 years ago by karsten

Resolution: fixed
Status: needs_reviewclosed

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!

Note: See TracTickets for help on using tickets.