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

Do not guess if a string is already hex encoded #216

Merged
merged 3 commits into from
Aug 2, 2017

Conversation

carver
Copy link
Collaborator

@carver carver commented Aug 2, 2017

Also, skip flake8 tests in several unnecessary folders. This significantly sped up my local flake8 and tox run.

What was wrong?

See issue #215

How was it fixed?

Stop inspecting the string for a leading '0x'

Cute Animal Picture

Cute animal picture

Also, skip flake8 tests in several unnecessary folders
@carver
Copy link
Collaborator Author

carver commented Aug 2, 2017

So I rebuilt my tox and the error started reproducing locally. There are a few libraries that changed between this and a recent successful:

  • certifi
  • hypothesis
  • pytest 3.1.3 to 3.2.0

Since the primary error is coming from an internal pytest plugin, I'm guessing it's the pytest upgrade. Investigating further...

@carver
Copy link
Collaborator Author

carver commented Aug 2, 2017

Confirmed that tox run is successful again after forcing pytest back to 3.1.3

(except the flake8 issue)

@carver
Copy link
Collaborator Author

carver commented Aug 2, 2017

Yup, 3.2.0 bug. Will be fixed in 3.2.1

Pinning to 3.1.8 for now, then we can go to >=3.2.1 after it's released.
Edit: Using their proposed workaround

@see pytest-dev/pytest#2644

No need to revert this when 3.2.1 is released, because it
provides a cleaner view in the pytest output
@@ -28,7 +28,9 @@
'0xbb9bc244d798123fde783fcc1c72d3bb8c189413',
],
),
)
),
ids=['nullbyte', 'soloaddr', 'addrlist']
Copy link
Member

Choose a reason for hiding this comment

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

Assuming this is the fix for pytest 3.2, I think I'd rather just pin the dependency to pytest<3.2.0 with a comment linking to the issue in the pytest repo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it fixes the pytest issue, but it also makes the pytest output somewhat more readable:

tests/contracts/test_normalization_of_return_types.py::test_normalizing_return_values[nullbyte] PASSED
tests/contracts/test_normalization_of_return_types.py::test_normalizing_return_values[soloaddr] PASSED
tests/contracts/test_normalization_of_return_types.py::test_normalizing_return_values[addrlist] PASSED

But I'm only mildly in favor of this approach. If you are more worried about consistency between tests than test output readability for this test, I'm happy to go the version pinning route.

Copy link
Member

Choose a reason for hiding this comment

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

I'm convinced. feel free to leave this in.

I don't know flake8 all that well, but clearly the intention
was to ignore py3 syntax errors here, and it does that now.

Plus, it's python2-only, so I didn't care enough to
investigate further
@pipermerriam pipermerriam merged commit 7dee518 into ethereum:master Aug 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants