Opened 4 months ago

Last modified 3 months ago

#33812 needs_review task

Add unit tests for bandwidth statistics functions

Reported by: MrSquanchee Owned by: MrSquanchee
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Minor Keywords: prop313, ipv6, outreachy-ipv6
Cc: teor Actual Points:
Parent ID: #33617 Points: 2
Reviewer: ahf Sponsor: Sponsor55-can

Description

We currently don't have tests for bandwidth statistics
reporting code.
We need to write tests for the following functions.

  1. commit_max()
  2. advance_obs()
  3. add_obs()
  4. rep_hist_bandwidth_assess()
  5. rep_hist_fill_bandwidth_history()
  6. rep_hist_get_bandwidth_lines()

This will be useful for #33617

Child Tickets

Change History (16)

comment:1 Changed 4 months ago by teor

Keywords: prop313 ipv6 outreachy-ipv6 added
Milestone: Tor: unspecified
Points: 2
Sponsor: Sponsor55-can
Version: Tor: unspecified

Adding various fields.

comment:2 Changed 4 months ago by MrSquanchee

Status: assignedneeds_review

Hii teor,
I made a pull request please checkout https://github.com/torproject/tor/pull/1853.
This is not complete but I want to know if I am going right early in time.
Please have a look at the PR, I have asked several questions there.

comment:3 Changed 4 months ago by teor

I had a quick look, and tried to answer your questions.

I'll do a more detailed review tomorrow or Wednesday.

comment:4 Changed 4 months ago by ahf

Reviewer: ahf

comment:5 Changed 4 months ago by ahf

Status: needs_reviewneeds_information

I think these patches are definitely a step in the right direction. The overall code seems sound.

What is the next step here? It sounds you want to do additional work here?

comment:6 in reply to:  5 Changed 4 months ago by MrSquanchee

Status: needs_informationassigned

Replying to ahf:

I think these patches are definitely a step in the right direction. The overall code seems sound.

Thanks. After everything is done, I need to make sure that these tests are exhaustive.

What is the next step here? It sounds you want to do additional work here?

I need to add tests for rep_hist_fill_bandwidth_history and rep_hist_get_bandwidth_lines().
And as teor said I have to refactor bandwidth code in a new file bw_array_st.h and bw_array_st.c.

Thanks,

Suraj Upadhyay (MrSquanchee).

comment:7 Changed 4 months ago by MrSquanchee

Status: assignedneeds_review

Hii Reviewer,
Can you help me with this CI fail ??

I added a new file bw_array_st.h and it
says there is no such file when including.

Apart from that. We are done with this ticket.

https://github.com/torproject/tor/pull/1853

comment:8 in reply to:  7 Changed 4 months ago by MrSquanchee

Replying to MrSquanchee:

Can you help me with this CI fail ??

I added a new file bw_array_st.h and it
says there is no such file when including.

Never mind, I figured it out :)
The pull request is all green now.
Waiting for suggestions.

comment:9 Changed 4 months ago by MrSquanchee

Hii teor and ahf,

It's been two weeks, and I think now is the perfect time to review this PR before
I forgot how did I do this :)

comment:10 Changed 4 months ago by teor

Thanks for your patience. We've been busy on other tasks for the last few weeks, but I'll remind ahf that this ticket needs to be reviewed.

comment:11 Changed 3 months ago by ahf

Status: needs_reviewneeds_revision

Sorry for the slow response here. I think the patches looks very good and I was only able to find a few smaller nitpicks in the code.

I think this is a big step in the right direction for this code.

comment:12 in reply to:  11 Changed 3 months ago by MrSquanchee

Status: needs_revisionneeds_review

Replying to ahf:

Sorry for the slow response here. I think the patches looks very good and I was only able to find a few smaller nitpicks in the code.

I think this is a big step in the right direction for this code.

Thanks, I moved out all of the function declarations from bw_array_st.h to bw_array.h.
And changed add_obs function to inline.

Reviewer changes implemented : https://github.com/torproject/tor/pull/1853

comment:13 Changed 3 months ago by MrSquanchee

In my latest commit I removed inlining from add_obs as it caused referencing problems
in the functions calling it.

Also I don't think inlining the said function would be a good idea.
Only small functions should be ideal for inlining though.

Thanks.

comment:14 Changed 3 months ago by ahf

Status: needs_reviewneeds_revision

The code is starting to look very good. I have added two smaller issues with the STATIC handling and a nitpick about a filename typo.

comment:15 Changed 3 months ago by teor

I also added some issues with the changes file, forward declarations, and duplicate or redundant code.

comment:16 Changed 3 months ago by MrSquanchee

Status: needs_revisionneeds_review

Thanks to both of you for your suggestions, I have added your suggestions in the PR.
I think, we are done with it now.

https://github.com/torproject/tor/pull/1853

Note: See TracTickets for help on using tickets.