Ticket30406 refactor header constants #357

juga0 · 2019-05-06T07:38:03Z

No description provided.

to start refactoring using stem's code and don't keep duplicated code. For now, including only the header constant.

Replace when possible, all header constants to use Stem's one, not changing functionality. Tests don't need modifications, since functionality isn't changed. Closes #30406

asn-d6 · 2019-05-10T10:10:26Z

sbws/lib/stem_bandwidth_file.py

@@ -0,0 +1,67 @@
+"""Stem's Bandwidth file parser part.


Hmm, is this the right way to do this? Just include a file from stem? Doesn't this mean we need to keep track of stem's revisions to this file? :/

Also if this is the right way to do this, then perhaps we should mention which revision of this file this is (commit id). Also, why does it say "To be removed on Stem's release 1.8.0"?

Hmm, is this the right way to do this? Just include a file from stem? Doesn't this mean we need to keep track of stem's revisions to this file? :/

Only until the next release, i think there're stem's releases every ~6 months.
And so far this file has only changed when after adding more headers to sbws i create an issue in stem so it gets added too.
With this change, instead i could directly create a PR in stem and duplicate it in sbws until the next release.

Not sure is there's a better way, but i definitely don't want to wait 8 months again to do a sbws release waiting for a stem release and it's not a solution to change the dependency to stem's git master because that does not work for system packages (Debian).

Also if this is the right way to do this, then perhaps we should mention which revision of this file this is (commit id).

Do you think it would be better i just add the stem's bandwidth_file.py as it is currently?.
I can add the commit id (in v3bwfile if i add the stem's bandwidth_file.py as it's)

Also, why does it say "To be removed on Stem's release 1.8.0"?

Because that's the next Stem's release, i can add this comment somewhere. Where it would fit better?.

if i add the file as it is, i also need to copy the __init__.py
Thinking more about this, i think it's better to leave only that part, to follow better what i substitute.
I could also do a try import and if there's no bandwidth_file.py in stem then leave the old code + the refactor.
But i think duplicating all that code when as i said, the file won't change unless someone open a ticket, and there'll be a new stem release soon.

See small changes in fixup

sbws/lib/v3bwfile.py

asn-d6 · 2019-05-10T10:12:27Z

sbws/lib/v3bwfile.py

+        # Initialize all defined attrs,
+        # XXX: Since using the stem's name for the attributes will require
+        # other refactoring here, use the name in the actual file.
+        header_keys = [k for k, _ in HEADER_ATTR.values()]


It's not easy to understand if this code is refactoring or changes the behavior of sbws (adds/removes headers).

Could we have a unittest which makes sure that the old code and the new code will output the exact same thing? Perhaps instead of commenting out the code above, you can move it to the tests directory and make a unittest that checks the output of the old with the new code. Would this make sense?

It's not easy to understand if this code is refactoring or changes the behavior of sbws (adds/removes headers).

Could we have a unittest which makes sure that the old code and the new code will output the exact same thing? Perhaps instead of commenting out the code above, you can move it to the tests directory and make a unittest that checks the output of the old with the new code. Would this make sense?

Makes sense but then need to keep this old code in the test.
Stem has already tests for all the bandwidth files versions using data generated from sbws.
I checked, and including the stem's tests would require adding many more files.

So i came out with other solution that it's something i wanted to do time ago: include real measurements and bandwidth files generated with the test network for the tests.

So i run the integration tests to generate those files with master branch, then i created a test that generates the bandwidth file from the previous results and compare that the bandwidth file is the same.

I'm not pushing these changes yet until knowing your opinion. If you don't like that solution will open other ticket to include them.

Other way of ensuring that i didn't accidentally change functionality is the fact that i didn't have to change any test for this PR. It's true though that i don't think there're unit tests checking all the headers, but the previous idea would solve that.

Pushed the new test

I like this new test very much. I think it's a good step.
I added a few comments while I was trying to understand what the test does.

to test generating a bandwidth file from a whole network measurements data.

asn-d6 · 2019-05-27T12:01:30Z

tests/data/.sbws.ini

+data_period = 1
+
+[paths]
+sbws_home = .sbws


Perhaps we don't need to hide this data in .sbws? I'm not used to files in source code being hidden. Why are we doing it here?

teor2345

This pull request creates a lot of technical debt.

Let's write the code for stem 1.7.0 and stem master now, and test it with stem 1.7.0 and stem master using CI.

teor2345 · 2019-05-27T11:48:28Z

sbws/lib/stem_bandwidth_file.py

+658dd5281604eb9c63a91e529501947ecc65ef6b, which will be included in the next
+Stem's release, 1.8.0, except ``_date`` because depends on other stem's module.
+"""
+# XXX: Remove this file when stem releases 1.8.0.


We can't remove this file when stem releases 1.8.0.

We can only remove this file when everyone who uses sbws 1.2.0 or later, has access to stem 1.8.0 or later. We will need to check Debian package versions for stem and sbws.

teor2345 · 2019-05-27T11:51:53Z

sbws/lib/v3bwfile.py

@@ -15,6 +19,11 @@
                          TORFLOW_SCALING, SBWS_SCALING, TORFLOW_BW_MARGIN,
                          TORFLOW_OBS_LAST, TORFLOW_OBS_MEAN,
                          PROP276_ROUND_DIG, MIN_REPORT, MAX_BW_DIFF_PERC)
+
+# XXX: replace this line by:


Don't write a comment, write code:

Suggested change

# XXX: replace this line by:

try:

from stem.descriptor.bandwidth_file import HEADER_ATTR, _int

except ImportError:

from .stem_bandwidth_file import HEADER_ATTR, _int

except:

raise

teor2345 · 2019-05-27T11:53:02Z

sbws/lib/v3bwfile.py

-# with the other KeyValues.
+
+# XXX: These constants need to be kept even if using Stem's header one.
+# Unless Stem's will add a property type to the attributes.


I don't understand this comment.
Please change the comment so it is clearer.

What does stem need to do?
Is there a stem ticket for this change?

teor2345 · 2019-05-27T11:54:24Z

sbws/lib/v3bwfile.py

+
+# XXX: The following constants are kept for compatibility with the generator/
+# parser here. Use stem's parser/generator when possible and refactor this
+# code when stem releases 1.8.0.


Don't write comments, write code.

Refactor the code now. Use the old code with old stem, and the new code with new stem.

Then you can write CI tests for sbws that use stem 1.7.0 and stem master.

teor2345 · 2019-05-27T11:55:08Z

sbws/lib/v3bwfile.py


 TERMINATOR = '====='

 # Bandwidth Lines KeyValues
 # =========================
 # Num header lines in v1.X.X using all the KeyValues
-NUM_LINES_HEADER_V1 = len(ALL_KEYVALUES) + 2
+NUM_LINES_HEADER_V1 = len(HEADER_ATTR) + 2


Why 2?
Please make 2 into a named constant.

teor2345 · 2019-05-27T11:56:16Z

sbws/lib/v3bwfile.py

@@ -250,20 +202,28 @@ class V3BWHeader(object):
          when the generator started
    """
    def __init__(self, timestamp, **kwargs):
+        # XXX: do not convert all the arguments to string here, convert them
+        # only in __str__ and check their actual types.


This comment explains what the code is doing.
But why are the types important?

teor2345 · 2019-05-27T11:57:39Z

sbws/lib/v3bwfile.py

+
+        # Initialize all defined attrs,
+        # XXX: Since using the stem's name for the attributes will require
+        # other refactoring here, use the name in the actual file.


What is "the name in the actual file"?
Do you mean "the key names from the bandwidth file spec"?

teor2345 · 2019-05-27T11:58:50Z

sbws/lib/v3bwfile.py

@@ -1056,6 +1017,8 @@ def is_max_bw_diff_perc_reached(bw_lines,
        sum_bw = sum([l.bw for l in bw_lines
                      if getattr(l, 'consensus_bandwidth', None)
                      and getattr(l, 'unmeasured', 0) == 0]) * 1000
+        # For tests without consensus bandwidth, avoid division by 0
+        sum_bw = sum_bw or 1


It is weird to see a test workaround in a code file.

Please separate the tests and the code.

teor2345 · 2019-05-27T12:00:15Z

sbws/lib/v3bwfile.py

+# XXX: replace this line by:
+# from stem.descriptor.bandwidth_file import HEADER_ATTR, _int
+# when stem releases 1.8.0. More information in stem_bandwidth_file.py
+from .stem_bandwidth_file import HEADER_ATTR, _int


Please write CI tests for sbws that use stem 1.7.0 and stem master.

asn-d6 · 2019-05-27T12:02:10Z

tests/data/.sbws/tor/cached-consensus

-valid-after 2019-03-25 13:08:50
-fresh-until 2019-03-25 13:09:00
-valid-until 2019-03-25 13:09:20
+valid-after 2019-05-11 06:32:10


Why did we need to update the consensus?

juga0 added 2 commits May 6, 2019 07:24

chg: Add stem's bandwidth_file.py part

8a5efb1

to start refactoring using stem's code and don't keep duplicated code. For now, including only the header constant.

chg: v3bwfile: !refactor. Replace constants

ebbb9ff

Replace when possible, all header constants to use Stem's one, not changing functionality. Tests don't need modifications, since functionality isn't changed. Closes #30406

asn-d6 reviewed May 10, 2019

View reviewed changes

sbws/lib/v3bwfile.py Outdated Show resolved Hide resolved

asn-d6 reviewed May 10, 2019

View reviewed changes

juga0 added 3 commits May 19, 2019 16:03

fixup! chg: Add stem's bandwidth_file.py part

c2dfe26

fixup! chg: v3bwfile: !refactor. Replace constants

2d92fbd

new: tests: Add test network data and v3bwfile test

9214877

to test generating a bandwidth file from a whole network measurements data.

juga0 force-pushed the ticket30406_refactor_header_constants branch from bb79a18 to 9214877 Compare May 19, 2019 16:56

fixup! new: tests: Add test network data and v3bwfile test

c3769b9

asn-d6 reviewed May 27, 2019

View reviewed changes

teor2345 suggested changes May 27, 2019

View reviewed changes

asn-d6 reviewed May 27, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ticket30406 refactor header constants #357

Ticket30406 refactor header constants #357

juga0 commented May 6, 2019

asn-d6 May 10, 2019

juga0 May 11, 2019

juga0 May 19, 2019

juga0 May 19, 2019

asn-d6 May 10, 2019

juga0 May 11, 2019

juga0 May 11, 2019

juga0 May 19, 2019

asn-d6 May 27, 2019

asn-d6 May 27, 2019

teor2345 left a comment

teor2345 May 27, 2019

teor2345 May 27, 2019 •

edited

Loading

teor2345 May 27, 2019

teor2345 May 27, 2019

teor2345 May 27, 2019

teor2345 May 27, 2019

teor2345 May 27, 2019

teor2345 May 27, 2019

teor2345 May 27, 2019

asn-d6 May 27, 2019

Ticket30406 refactor header constants #357

Are you sure you want to change the base?

Ticket30406 refactor header constants #357

Conversation

juga0 commented May 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 May 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 May 27, 2019 •

edited

Loading