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

Skip test_source_mtime_long_long on 32bit and lower platforms #5045

Merged
merged 1 commit into from
May 7, 2019
Merged

Skip test_source_mtime_long_long on 32bit and lower platforms #5045

merged 1 commit into from
May 7, 2019

Conversation

mimi1vx
Copy link
Contributor

@mimi1vx mimi1vx commented Apr 4, 2019

  • Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
  • Target the master branch for bug fixes, documentation updates and trivial changes.
  • Target the features branch for new features and removals/deprecations.
  • Include documentation when adding new features.
  • Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

  • [x ] Add yourself to AUTHORS in alphabetical order;

@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #5045 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5045      +/-   ##
=========================================
- Coverage   96.06%     96%   -0.07%     
=========================================
  Files         114     114              
  Lines       25768   25769       +1     
  Branches     2548    2548              
=========================================
- Hits        24754   24739      -15     
- Misses        704     715      +11     
- Partials      310     315       +5
Impacted Files Coverage Δ
testing/test_assertrewrite.py 84.06% <100%> (+0.02%) ⬆️
src/_pytest/assertion/util.py 93.38% <0%> (-4.29%) ⬇️
src/_pytest/unittest.py 93.12% <0%> (-1.06%) ⬇️
src/_pytest/doctest.py 95.69% <0%> (-0.72%) ⬇️
testing/test_pdb.py 98.99% <0%> (-0.21%) ⬇️

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 973301b...77526f4. Read the comment docs.

blueyed
blueyed previously approved these changes Apr 4, 2019
@blueyed
Copy link
Contributor

blueyed commented Apr 4, 2019

Thanks!

The test was added for #4903 - /cc @bmwiedemann

Just for future reference: you should use "Fixes …" in commit message, especially given that you have created an issue already.

@bmwiedemann
Copy link
Contributor

hmm. Wasnt there this size & 0xffffffff that is supposed to handle overly large files? If something is broken there, there should be a better fix than this.

@blueyed blueyed dismissed their stale review April 4, 2019 14:36

waiting for feedback

@mimi1vx
Copy link
Contributor Author

mimi1vx commented Apr 5, 2019

@bmwiedemann python Integer bigger that 2^31-1 can't be converted to 'int' on a platform where sys.maxsize is 2^31-1 .. so this test is wrong on this platforms, and proprely cast OverflowError Exception

@bmwiedemann
Copy link
Contributor

The test is not doing a cast to int. And the code specifically uses "L" for unsigned to have a 0..2^32-1 range. Maybe including a backtrace would make the problem clearer.

@nicoddemus
Copy link
Member

The traceback was included in #5046 (comment):

[  348s]         timestamp = 2 ** 32 + offset
[  348s] >       os.utime(str(p), (timestamp, timestamp))
[  348s] E       OverflowError: Python int too large to convert to C long

And seems to be failing for both -1 and +1 offsets.

Strangely, this passes for me on 32-bit Python 2.7 on Windows:

λ python -c "import platform;print platform.architecture()"
('32bit', 'WindowsPE')

λ pytest testing\ -k test_source_mtime_long_long -v
======================== test session starts ========================
platform win32 -- Python 2.7.13, pytest-4.4.1.dev51+g1ed4ae863, py-1.6.0, pluggy-0.9.0
...

testing/test_assertrewrite.py::test_source_mtime_long_long[-1] PASSED [ 50%]
testing/test_assertrewrite.py::test_source_mtime_long_long[1] PASSED [100%]

============= 2 passed, 2383 deselected in 3.30 seconds =============

On 64-bits Python I get, as expected:

λ py -3.6 -c "import platform;print(platform.architecture())"
('64bit', 'WindowsPE')

I would expect to be able to reproduce this...

@nicoddemus
Copy link
Member

@mimi1vx any updates on this? Can you please post the output of:

λ python -c "import platform;print platform.architecture()"
('32bit', 'WindowsPE')

In your system?

@nicoddemus nicoddemus added the status: needs information reporter needs to provide more information; can be closed after 2 or more weeks of inactivity label May 4, 2019
@nicoddemus nicoddemus changed the title Skip test_source_mtime_long_long on 32bit and lower platforms WIP Skip test_source_mtime_long_long on 32bit and lower platforms May 4, 2019
@mimi1vx
Copy link
Contributor Author

mimi1vx commented May 7, 2019

@nicoddemus

[    3s] + python2 -c 'import platform;print platform.architecture()'
[    3s] ('32bit', 'ELF')

@nicoddemus
Copy link
Member

@mimi1vx interesting, thanks. Which OS is that?

@mimi1vx
Copy link
Contributor Author

mimi1vx commented May 7, 2019

@nicoddemus ELF - > linux

btw>

abuild@penguin:~> python2 
Python 2.7.16 (default, Mar 04 2019, 07:13:50) [GCC] on linux2
>>> timestamp = 2 ** 32 +1
>>> import os
>>> os.utime(str("/etc/os-release"), (timestamp, timestamp))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: Python int too large to convert to C long
abuild@penguin:~> python3
Python 3.7.2 (default, Dec 30 2018, 16:18:15) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> timestamp = 2 ** 32 +1
>>> os.utime(str("/etc/os-release"), (timestamp, timestamp))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OverflowError: timestamp out of range for platform time_t
>>> 

@nicoddemus
Copy link
Member

I see, thanks!

Well now that's been clarified, I think it is harmless to merge this, otherwise the test blows up anyway. 👍

@nicoddemus nicoddemus changed the title WIP Skip test_source_mtime_long_long on 32bit and lower platforms Skip test_source_mtime_long_long on 32bit and lower platforms May 7, 2019
@nicoddemus nicoddemus merged commit ef4dec0 into pytest-dev:master May 7, 2019
@mimi1vx mimi1vx deleted the 32bit branch May 7, 2019 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs information reporter needs to provide more information; can be closed after 2 or more weeks of inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants