[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #19259 [Metrics/Onionoo]: uncaught NFE

#19259: uncaught NFE
-----------------------------+--------------------------------
 Reporter:  iwakeh           |          Owner:  iwakeh
     Type:  defect           |         Status:  needs_revision
 Priority:  High             |      Milestone:
Component:  Metrics/Onionoo  |        Version:
 Severity:  Major            |     Resolution:
 Keywords:                   |  Actual Points:
Parent ID:                   |         Points:
 Reviewer:                   |        Sponsor:
-----------------------------+--------------------------------
Changes (by karsten):

 * status:  needs_information => needs_revision


Comment:

 Sorry for the long delay in responding.  This patch is not easy to review
 for me, because we're using somewhat different approaches to writing unit
 tests.  Can we discuss those approaches first before going into the
 details of reviewing and merging this particular test class?  I think
 there's a benefit for future test classes here.

  - I'm having a hard time connecting the test data at the beginning of the
 class with the methods where they're used.  I see how it's useful to
 separate the two, but may I suggest that test data preceding actual tests
 should be self-explanatory without having to read subsequent tests?  For
 example, the name `compareLines` doesn't really make much sense to me
 without reading the corresponding test method.  And `correctLines` and
 `correctHistory` could be described a little bit better.
  - I prefer tests to be as small as possible and not mix different test
 scenarios.  For example, rather than stuffing four (related) test input
 lines into `wrongDateLines` and commenting them there, I'd probably write
 four test methods with self-explanatory method names that simply include
 the test data themselves.  That would also solve the previous aspect where
 test data preceding test methods is not self-explanatory to the reader.
  - Most of the test data lines violate the code style by being longer than
 70 or 80 or whatever we said characters.  And I think there should be a
 space in `new String[]{`, but I'm not sure.  And there are missing
 newlines between variable declarations.
  - To me, test method names should be self-explanatory, even if that makes
 them rather lengthy.  Rather than `testSetup`, I'd prefer something like
 `testEmptyWeightsStatus`.

 There, these are some of the things I'd do differently.  I can be
 convinced that your approach has advantages over mine.  Or maybe some of
 these suggestions make sense to you and you can send me an updated patch?
 Happy to look again, and, as always, thanks for your hard work!

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/19259#comment:11>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs