Skip to content
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

Port test_introducer.py to Python 3 #905

Merged
merged 16 commits into from Dec 2, 2020
Merged

Conversation

itamarst
Copy link
Collaborator

Copy link
Member

@exarkun exarkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks pretty good. Some comments inline. Here's N more:

  • Do we have a best practice for strings that represent paths? The first two tests in
    test_introducer make two different choices - native string literal and unicode string literal.
  • in test_introducer there is a TooNewServer that still uses native strings for its VERSION. It's used by a test that asserts some error has happened. I guess that test is trying to assert that the right error happened but since the real IntroducerService has had its VERSION made into byte strings instead of native strings I wonder if that test is really working right.

src/allmydata/introducer/client.py Outdated Show resolved Hide resolved
src/allmydata/introducer/server.py Outdated Show resolved Hide resolved
src/allmydata/test/common.py Outdated Show resolved Hide resolved
src/allmydata/test/test_introducer.py Show resolved Hide resolved
src/allmydata/test/test_introducer.py Show resolved Hide resolved
@itamarst
Copy link
Collaborator Author

I could not reproduce the Python 3 failure locally, but tried to fix it based on logs. Merged forward, hopefully correctly.

Copy link
Member

@exarkun exarkun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks good to me. Just one minor comment inline. Please address and then merge.

src/allmydata/introducer/client.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 1, 2020

Codecov Report

Merging #905 (5872220) into master (fba386c) will increase coverage by 0%.
The diff coverage is 95%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #905   +/-   ##
======================================
  Coverage      92%     92%           
======================================
  Files         157     157           
  Lines       27419   27436   +17     
  Branches     4087    4093    +6     
======================================
+ Hits        25188   25216   +28     
+ Misses       1569    1560    -9     
+ Partials      662     660    -2     
Impacted Files Coverage Δ
src/allmydata/util/_python3.py 100% <ø> (ø)
src/allmydata/introducer/common.py 98% <86%> (-2%) ⬇️
src/allmydata/introducer/client.py 91% <94%> (+<1%) ⬆️
src/allmydata/client.py 94% <100%> (+<1%) ⬆️
src/allmydata/introducer/server.py 96% <100%> (+<1%) ⬆️
src/allmydata/mutable/publish.py 97% <100%> (+1%) ⬆️
src/allmydata/web/introweb.py 99% <100%> (ø)
src/allmydata/web/status.py 95% <0%> (+<1%) ⬆️
src/allmydata/util/rrefutil.py 92% <0%> (+12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fba386c...5872220. Read the comment docs.

@itamarst itamarst merged commit 71d287c into master Dec 2, 2020
@itamarst itamarst deleted the 3514.test-introducer-python-3 branch December 2, 2020 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants