New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python3 unit test fixes pt1 #6698

Merged
merged 6 commits into from Oct 29, 2018

Conversation

Projects
None yet
3 participants
@OniOni
Contributor

OniOni commented Oct 29, 2018

Problem

We have a couple unit tests that fail when run against python 3.

Solution

Fix tests.

Result

This PR contains fixes for 4 tests:

- test_resolve_multiplatform_requirements
- test_resolve_simple_requirements
- test_walk_graph
- test_upload_stats

More tests where fixed by earlier work, but not uncovered because of blacklist.

(this is a cleaned up version of the work @ #6682)

@OniOni OniOni referenced this pull request Oct 29, 2018

Closed

Python3 test collecting #6682

@Eric-Arellano

Wow, down to only 2 unit tests! This is so exciting to see :)

@@ -127,7 +127,7 @@ def reset(self):
"""
self._target_by_address = OrderedDict()
self._target_dependencies_by_address = defaultdict(OrderedSet)
self._target_dependees_by_address = defaultdict(set)
self._target_dependees_by_address = defaultdict(OrderedSet)

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Oct 29, 2018

Contributor

Weird we used set here when we used OrderedSet right above. Looks like a good change.

@@ -37,6 +37,7 @@ def do_POST(handler):
decoded_post_data = {k: json.loads(v[0]) for k, v in post_data.items()}
self.assertEqual(stats, decoded_post_data)
handler.send_response(200)
handler.end_headers()

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Oct 29, 2018

Contributor

This is a great find. Well done!

@Eric-Arellano Eric-Arellano requested a review from stuhood Oct 29, 2018

@Eric-Arellano

This comment has been minimized.

Contributor

Eric-Arellano commented Oct 29, 2018

One potential concern, have you run the newly unblacklisted tests multiple times to test for non-determinism? I remember one of my changes over the summer slipped into master because it only failed half the time.

@OniOni

This comment has been minimized.

Contributor

OniOni commented Oct 29, 2018

Yeah, I've run them a couple times and it seems okay (ci and locally).
Always tricky with non-determinism.
The good thing is most of my tests are either relaxing tests or tightening non-deterministic behavior. So 🤞

@stuhood

Thanks!

@stuhood

This comment has been minimized.

Member

stuhood commented Oct 29, 2018

The CI failure is a flaky test. Merging.

@stuhood stuhood merged commit b7de5f7 into pantsbuild:master Oct 29, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment