Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16299 closed defect (fixed)

Stem unit test failure for test_compare_flags

Reported by: atagar Owned by: atagar
Priority: Low Milestone:
Component: Archived/Stem Version:
Severity: Keywords: testing
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

On irc toralf mentioned having the following unit test failure. It doesn't fail for me so maybe a missing mock...

======================================================================
FAIL: test_compare_flags
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib64/python3.3/unittest/mock.py", line 1087, in patched
    return func(*args, **keywargs)
  File "/home/tfoerste/devel/stem/test/unit/tutorial_examples.py",
line 280, in test_compare_flags
    self.assert_equal_unordered(COMPARE_FLAGS_OUTPUT,
stdout_mock.getvalue())
  File "/home/tfoerste/devel/stem/test/unit/tutorial_examples.py",
line 141, in assert_equal_unordered
    self.assertCountEqual(expected.splitlines(), actual.splitlines())
AssertionError: Element counts were not equal:

Diff is 1209 characters long. Set self.maxDiff to None to see it.

----------------------------------------------------------------------
Ran 6 tests in 0.043s

Child Tickets

Attachments (2)

0001-Ensure-the-compare_flags-test-gives-reliable-results.patch (3.0 KB) - added by cypherpunks 5 years ago.
0001-Ensure-the-compare_flags-test-gives-reliable-results.2.patch (2.7 KB) - added by cypherpunks 5 years ago.
Patch version 2 using an ordered dictionary

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by atagar

Sounds from #16390 like this is no longer an issue. Feel free to reopen if that's not the case.

comment:2 Changed 5 years ago by atagar

Looks like I failed to close this. Just as well, toralf reports that he's seeing it again.

comment:3 Changed 5 years ago by toralf

Well, the culprint might be related to the following :
With

/home/tfoerste/devel/stem/run_tests.py --unit  --test tutorial  --log TRACE

python-2.7.9 gives

...
  650 CIRC 10 BUILT $B1FA7D51B8B6F0CB585D944F450E7C06EDE7E44C=ByTORAndTheSnowDog,$00C2C2A16AEDB51D5E5FB7D6168FC66B343D822F=ph3x,$65242C91BFF30F165DA4D132C81A9EBA94B71D62=torexit16 PURPOSE=GENERAL
()

whereas 3.3.5 lacks the empty array:

  650 CIRC 10 BUILT $B1FA7D51B8B6F0CB585D944F450E7C06EDE7E44C=ByTORAndTheSnowDog,$00C2C2A16AEDB51D5E5FB7D6168FC66B343D822F=ph3x,$65242C91BFF30F165DA4D132C81A9EBA94B71D62=torexit16 PURPOSE=GENERAL

comment:4 Changed 5 years ago by cypherpunks

Status: newneeds_review

I believe this patch fixes the issue. See the commit message for more information.

One thing i forgot to mention in the commit message is the reason why the issue does not occur in the get_authorities loop. The return value of get_authorities call is mocked to a list which guarantees the ordering.

comment:5 Changed 5 years ago by atagar

Hi cypherpunks, great catch! However, I'm a bit concerned that we're making the compare_flags.py example considerably less performant. We issued queries in parallel, but now it's serial. Can we fix the test without slowing down the example?

comment:6 in reply to:  5 Changed 5 years ago by cypherpunks

Replying to atagar:

Hi cypherpunks, great catch! However, I'm a bit concerned that we're making the compare_flags.py example considerably less performant. We issued queries in parallel, but now it's serial. Can we fix the test without slowing down the example?

Can you explain why you think the patch changes the execution from parallel to serial? It still calls run() for every query which AFAICT spawns threads.

The patch moves the run() calls into the get_authorities loop which removes one for loop. Therefore, i expect the patch to improve performance even though this wasn't the goal of the patch (nor this ticket).

comment:7 Changed 5 years ago by atagar

Ahh, think this might be confusion because of the method name I chose. Calling get_vote() kicks off a request and provides a Query instance, and run() blocks until you get the result. Think of it like a join() in terms of threads. So previously we had...

for each authority:
  request the vote

for each request:
  wait for the response

... and is now...

for each authority:
  request the vote and await the reply

Changed 5 years ago by cypherpunks

Patch version 2 using an ordered dictionary

comment:8 in reply to:  7 Changed 5 years ago by cypherpunks

Replying to atagar:

Ahh, think this might be confusion because of the method name I chose. Calling get_vote() kicks off a request and provides a Query instance, and run() blocks until you get the result.

After rereading the code i see queries start on initialization. I missed that the last time and thought run() starts the queries.

Because of this flaw you can disregard the first patch. The second patch uses an ordered dictionary which ensures queries.items() returns key/value pairs in the same order they were added. The same order meaning the order in which the authorities are returned by get_authorities.

comment:9 Changed 5 years ago by atagar

Resolution: fixed
Status: needs_reviewclosed

Thanks! Pushed. I might need to tweak it a tad around release time to work with python 2.6 (which lacks OrderedDict, we have a stem.util.ordereddict for that reason), but maybe it's time to drop support for the version. I've been temped to for quite a while. :P

comment:10 Changed 5 years ago by cypherpunks

PEP 361 mentions

Python 2.6.9 is the final security-only source-only maintenance
release of the Python 2.6 series.  With its release on October 29,
2013, all official support for Python 2.6 has ended.  Python 2.6
is no longer being maintained for any purpose.

So i am thinking it is time.

comment:11 Changed 5 years ago by atagar

Gotcha. The main reason we've kept it is because of Redhat, who's glacial pace makes even Debian look like a zippy early adopter. At present I don't have a compelling reason to drop python 2.6 (we could drop a few hacks, but 2.6 support isn't harming anything). That said, if there ever is a half decent reason to axe it then it's going away. ;)

At present I'm thinking 'keep it until either the Stem 2.0.0 release or we have a compelling reason to drop it'.

Note: See TracTickets for help on using tickets.