Opened 6 months ago

Last modified 4 months ago

#25947 merge_ready enhancement

Create unit test for dirserv_read_measured_bandwidths

Reported by: juga Owned by: juga
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: bwauth, test, tor-bwauth
Cc: teor Actual Points:
Parent ID: #25925 Points:
Reviewer: asn Sponsor:

Description

So that we ensure that tor will still continue to read V3BandwidthsFile documents when we add new headers to it in version 1.1.0, as proposed in #25869

Child Tickets

Change History (21)

comment:2 Changed 6 months ago by teor

Keywords: test added
Milestone: Tor: 0.3.4.x-final

This test is waiting on spec updates in:
https://github.com/torproject/torspec/pull/4

Let us know when it's ready for review.

comment:3 Changed 6 months ago by juga

Parent ID: #25925

comment:4 Changed 6 months ago by juga

Added additional lines to test in https://github.com/torproject/tor/pull/77
Header is still not being tested.

comment:5 Changed 6 months ago by juga

To test the header, would it be possible to refactor the code as proposed in #25960, or there should not be any change to the code here?

comment:6 Changed 6 months ago by teor

You can read my response your question at:
https://github.com/torproject/tor/pull/78#issuecomment-386196368

Here's how we can change the code safely:

  • make a commit with unit tests for the relay lines
  • make a commit that refactors the code so you can test a whole file
  • make a commit that unit tests a whole torflow file
  • make changes to accept the new headers
  • make a commit that unit tests a whole sbws file

comment:7 Changed 5 months ago by juga

make a commit with unit tests for the relay lines

That's https://github.com/torproject/tor/pull/77, i'll assume i should wait for the rest of changes to review

comment:8 Changed 5 months ago by juga

And https://github.com/torproject/tor/pull/87, same but based on maint-0.2.9

comment:9 Changed 5 months ago by nickm

Status: assignedneeds_review

(Should this be in needs_review? Assuming so.)

comment:10 in reply to:  7 Changed 5 months ago by teor

Status: needs_reviewneeds_information

Replying to juga:

make a commit with unit tests for the relay lines

That's https://github.com/torproject/tor/pull/77, i'll assume i should wait for the rest of changes to review

We're waiting on the final spec.

comment:11 Changed 5 months ago by juga

We're waiting on the final spec.

Even if we go for 2.0.0 (without passing by 1.1.0 first),
the old format should still be backported, and it does not matter whether the additional
header lines are KeyValue or use SP

~make a commit with unit tests for the relay lines~

This is already done by test_dir_measured_bw_kb.
For the test where node_id failed to be at the end, it's done in #26004, commit dbc80ad19b8a27aab557fe7930e18eef8f415133

~make a commit that refactors the code so you can test a whole file~

After nickm pointed how to create temporal files for tests in https://trac.torproject.org/projects/tor/ticket/26007#comment:11, this is not longer needed

~make a commit that unit tests a whole torflow file~

commit in branch ticket25947_029_squashed
https://github.com/juga0/tor/commit/ea0e12ef31c157d43c949dda1469b43704836322

~make changes to accept the new headers~

This was technically #25960.

commit in branch ticket25947_029_squashed
https://github.com/juga0/tor/commit/363cbb4196da91d63c3ff93319911636611018dc

~make a commit that unit tests a whole sbws file~

commit in branch ticket25947_029_squashed
https://github.com/juga0/tor/commit/789f794f9fcbc37870e55de3364b3e1dee7dc22a

comment:12 Changed 5 months ago by juga

Status: needs_informationneeds_review

comment:13 Changed 5 months ago by dgoulet

Reviewer: asn

comment:14 Changed 5 months ago by asn

Status: needs_reviewneeds_revision

Second round of review (first one was over IRC):

  • There is at least one more memleak in the unittests. Example:
    +  tor_asprintf(&content, "%ld\n%s%s", timestamp, v110_header_lines,
    +               sbws_relay_lines);
    +  write_str_to_file(fname, content, 0);
    +  log_debug(LD_DIRSERV, "Bandwidth list file with additional headers and "
    +            "sbws RelayLine.");
    +  tt_int_op(0, OP_EQ, dirserv_read_measured_bandwidths(fname, NULL));
    +
    + done:
    +  teardown_capture_of_logs();
    +}
    
    Please use ASAN or valgrind to ensure that all memleaks are fixed. Check out doc/HACKING/HelpfulTools.md on how to use valgrind.
  • Commit 2e8c0b9054 adds header_ended argument to measured_bw_line_parse() but does not document what it does in the function doc. Also the name header_ended is a bit weird, what does it mean?
  • It's also not entirely clear what header_ended is supposed to do in dirserv_read_measured_bandwidths(). Why do we change it to 1? Perhaps a comment would be helpful.
  • There is trailing whitespace around the new code. Please use make check-spaces to fix that. It must be clean before merging.
  • I think the changelog entries are too detailed and also too vague at the same time. Example of vagueness:
    +  o Minor feature (unit tests):
    +    - Test that incomplete lines only give warnings when the end of the header
    +      has not been detected
    
    Incomplete lines of what? There is no mention of bwauths in the changes entry.

IMO, we should just add a single entry under Minor feature (unit tests): that says Improve bandwidth measurement unittests and that should be enough. Anyone who cares about the specific unittests, can just look at the commit themselves.

comment:15 Changed 5 months ago by juga

Solved those isues in ticket25947_032_03_refactor branch.

comment:16 Changed 5 months ago by juga

Status: needs_revisionneeds_review

comment:17 Changed 5 months ago by asn

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

Pushing to 035 along with #25960.

comment:18 Changed 5 months ago by asn

Status: needs_reviewneeds_revision

Hello,

I pushed a branch ticket25947_032_03_refactor with a few changes:
a) Renamed end_header variable to line_is_after_headers to follow the boolean variable convention of tor. Also clarified a few comments.
b) Fixed some trailing whitespace and double lines that were making make check-spaces complain.
c) Slightly edited the changes file to follow usual convention.

Juga, if you like the changes, please squash them into your branch (so that in the end you only have 4 commits) and let's turn this to merge_ready.

Also, please test this branch against torflow and swbws to make sure sbws to make sure that it functions properly. Let me know if you can't do this and I will do it if needed.

comment:19 Changed 5 months ago by juga

Status: needs_revisionmerge_ready

I agree with the changes and have squashed them in my ticket25947_032_03_refactor branch.

I've run the branch using torflow and sbws generated bandwidth file, and there was no any warnings (grep -r warn /pathtotorlogs/*.log|grep -i bandwidth).

Probably this should be backported to 032-backport, as #25960

comment:20 Changed 4 months ago by juga

Keywords: tor-bwauth added

comment:21 Changed 4 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.3.x-final

Merged to 0.3.4 and forward; marking for possible backport.

Note: See TracTickets for help on using tickets.