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

Fix tests for non x86 64 #176

Merged
merged 4 commits into from
Aug 13, 2019
Merged

Conversation

dcermak
Copy link
Contributor

@dcermak dcermak commented Aug 9, 2019

When enabling the test suite of python-packaging for opensuse, a small number of test failures popped up due to hardcoded values (e.g. "linux_x86_64" as the os arch) or wrong & untested assumptions (e.g. the test failure of test_macos_arch_detection on 32 bit architectures).

This PR should fix these issues for ARM, PPC and s390x.

Furthermore, the detection whether the current OS is a 64 bit one was flawed (it always returned False) and upon fixing that test coverage dropped. Therefore I have added the test test_linux_platforms_manylinux_unsupported to get test coverage back to 100%.

Theses tests are implicitly assuming that they are being run on x86_64 or i686,
but fail on ARM, PPC, etc.
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Two very minor changes relating to docstrings (basically they aren't needed and this code base generally doesn't worry about them for non-public code), but otherwise the code looks good!

tests/test_tags.py Outdated Show resolved Hide resolved
tests/test_tags.py Outdated Show resolved Hide resolved
@brettcannon
Copy link
Member

This PR LGTM, but I'll give other project owners a chance to review since this would be the PR I merged on this repo (plus it gives @dcermak a chance to remove the docstrings).

@pradyunsg
Copy link
Member

pradyunsg commented Aug 11, 2019

LGTM.

@brettcannon I'd merge this but I'm not gonna steal your merge from you. 🙃

distutils.util.get_platform() returns "linux-x86_64" on 64 bit Linux and not
"linux_86_64" as assumed by this function. Instead we use the first element
returned by platform.architecture() and move the check into a separate fixture.

Furthermore, we have to check whether the current OS is x86-based, as the
results don't match otherwise.
the else: branch was not covered in tags._linux_platforms() due to the from the
previous commit
tags._mac_arch always returns i386 on 32 bit Linux and thereby
tags._mac_platforms()[0] ends with "i386" even in the arch="x86_64" case
@dcermak
Copy link
Contributor Author

dcermak commented Aug 11, 2019

@brettcannon I've removed the pointless doc-strings.

@brettcannon brettcannon merged commit c429eed into pypa:master Aug 13, 2019
@brettcannon
Copy link
Member

Thanks, @dcermak !

@dcermak dcermak deleted the fix_tests_for_non_x86_64 branch August 13, 2019 21:23
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.

3 participants