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

gh-108996: add tests for msvcrt #109004

Merged
merged 13 commits into from Sep 8, 2023
Merged

gh-108996: add tests for msvcrt #109004

merged 13 commits into from Sep 8, 2023

Conversation

aisk
Copy link
Member

@aisk aisk commented Sep 6, 2023

@bedevere-bot bedevere-bot added awaiting review tests Tests in the Lib/test dir labels Sep 6, 2023
@terryjreedy
Copy link
Member

Pending review, this can be merged since the test_threading failure is not related. (It has failed elsewhere today, including on current main on my machine.) I will look at the patch.

@terryjreedy terryjreedy requested a review from a team September 6, 2023 17:00
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

It would be nice if one of the Windows experts looked at this, especially the tests of file operations (which I have no familiarity with).

Lib/test/test_msvcrt.py Outdated Show resolved Hide resolved
Lib/test/test_msvcrt.py Show resolved Hide resolved
Lib/test/test_msvcrt.py Outdated Show resolved Hide resolved
Lib/test/test_msvcrt.py Outdated Show resolved Hide resolved
Lib/test/test_msvcrt.py Outdated Show resolved Hide resolved
Lib/test/test_msvcrt.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

aisk and others added 9 commits September 7, 2023 16:28
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Copy link
Member

@zooba zooba left a comment

Choose a reason for hiding this comment

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

Seems okay to me. Passing tests are better than no tests at all.

Lib/test/test_msvcrt.py Outdated Show resolved Hide resolved
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
@terryjreedy terryjreedy added needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Sep 8, 2023
@terryjreedy terryjreedy merged commit bcb2ab5 into python:main Sep 8, 2023
25 checks passed
@miss-islington
Copy link
Contributor

Thanks @aisk for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 8, 2023
(cherry picked from commit bcb2ab5)

Co-authored-by: AN Long <aisk@users.noreply.github.com>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 8, 2023
(cherry picked from commit bcb2ab5)

Co-authored-by: AN Long <aisk@users.noreply.github.com>
Co-authored-by: Terry Jan Reedy <tjreedy@udel.edu>
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows10 3.x has failed when building commit bcb2ab5.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/146/builds/6284) and take a look at the build logs.
  4. Check if the failure is related to this commit (bcb2ab5) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/146/builds/6284

Failed tests:

  • test_msvcrt

Failed subtests:

  • test_kbhit - test.test_msvcrt.TestConsoleIO.test_kbhit
  • test_getwch - test.test_msvcrt.TestConsoleIO.test_getwch

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\test_msvcrt.py", line 65, in test_kbhit
    self.assertEqual(msvcrt.kbhit(), 0)
AssertionError: 1 != 0


Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\test_msvcrt.py", line 77, in test_getwch
    self.assertEqual(msvcrt.getwch(), c)
AssertionError: '\n' != '\u5b57'
+ \u5b57
- 
- 

@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

This change broke "AMD64 Windows10 3.x" buildbot and the Windows x64 CI job on GitHub Action :-(

buildbot failure: https://buildbot.python.org/all/#builders/146/builds/6284

GHA failure: https://github.com/python/cpython/actions/runs/6127105160/job/16632346105?pr=109168

FAIL: test_getwch (test.test_msvcrt.TestConsoleIO.test_getwch)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_msvcrt.py", line 77, in test_getwch
    self.assertEqual(msvcrt.getwch(), c)
AssertionError: '\n' != '\u5b57'
+ \u5b57
- 

@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

The GitHub Action job also started to log a new error:

ResourceUnavailable: D:\a\_temp\0ad13265-0ee9-4c5f-87fb-4d47fe5a3611.ps1:2
Line |
   2 |  .\PCbuild\rt.bat -p x64 -d -q -uall -u-cpu -rwW --slowest --timeout=1 �
     |  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | The Win32 internal error "No process is on the other end of the pipe." 0xE9 occurred while setting the console window title. Contact Microsoft Customer Support Services.

Error: Process completed with exit code 1.

@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

"AMD64 Windows10 3.x" buildbot failures:

======================================================================
FAIL: test_getwch (test.test_msvcrt.TestConsoleIO.test_getwch)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\test_msvcrt.py", line 77, in test_getwch
    self.assertEqual(msvcrt.getwch(), c)
AssertionError: '\n' != '\u5b57'
+ \u5b57
- 
- 


======================================================================
FAIL: test_kbhit (test.test_msvcrt.TestConsoleIO.test_kbhit)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\buildarea\3.x.bolen-windows10\build\Lib\test\test_msvcrt.py", line 65, in test_kbhit
    self.assertEqual(msvcrt.kbhit(), 0)
AssertionError: 1 != 0

@vstinner vstinner removed needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Sep 8, 2023
@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

vstinner removed needs backport to 3.11 needs backport to 3.12 labels

I disabled backport, to avoid backporting a change which breaks the CI.

@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

Ah, another strange thing in the GHA job:

FAILED (failures=1)
test test_msvcrt failed
0:16:43 load avg: 14.11 [327/463/2] test_devpoll process crashed (Exit code 3221225794) -- running: test_regrtest (2 min 58 sec), test_fstring (32.5 sec)

== Tests result: FAILURE ==

I don't know why test_devpoll is run on Windows, nor why it does crash :-( The test starts with:

if not hasattr(select, 'devpoll') :
    raise unittest.SkipTest('test works only on Solaris OS family')

But it's a bug in libregrtest which reports a test_msvcrt crash as a test_devpoll crash?

@terryjreedy
Copy link
Member

Backports are #109166 and #109167

@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

Backports are #109166 and #109167

I close them to prevent merging them by mistake. I would prefer to see two broken CI being fixed yet.

The regression is preventing to merge any new PR in main, since the Windows x64 job now fails :-(

@terryjreedy
Copy link
Member

Funny, it passed above. Feel free to revert: I don't know how.

@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

Funny, it passed above.

I don't know why it passed before!? It's surprising :-( At least, we know a buildbot which should be tested if a fix is proposed.

@vstinner
Copy link
Member

vstinner commented Sep 8, 2023

Feel free to revert: I don't know how.

I don't think that it would be productive to revert. Instead, I wrote PR #109169 to keep the test but always skip the test (on all platforms) for now, until someone is available to fix it.

@vstinner
Copy link
Member

vstinner commented Sep 9, 2023

Instead, I wrote PR #109169 to keep the test but always skip the test (on all platforms) for now, until someone is available to fix it.

Merged.

The strange part is that I re-ran Windows x64 job on my PR #109168 before PR #109169 was merged, and all tests pass Windows x64!

It seems like test_msvcrt is not reliable :-(

@vstinner
Copy link
Member

vstinner commented Sep 9, 2023

Similar failure on "AMD64 Windows11 Non-Debug 3.x" buildbot:

FAIL: test_getwch (test.test_msvcrt.TestConsoleIO.test_getwch)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\test\test_msvcrt.py", line 77, in test_getwch
    self.assertEqual(msvcrt.getwch(), c)
AssertionError: '\n' != '\u5b57'
+ \u5b57
- 
- 


======================================================================
FAIL: test_kbhit (test.test_msvcrt.TestConsoleIO.test_kbhit)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\test\test_msvcrt.py", line 65, in test_kbhit
    self.assertEqual(msvcrt.kbhit(), 0)
AssertionError: 1 != 0

logs: https://buildbot.python.org/all/#/builders/914/builds/2794

@vstinner
Copy link
Member

vstinner commented Sep 9, 2023

@terryjreedy: if you want, next week if can have a look at the failing tests. But when I ran the test manually on my Windows VM: oops, it just worked. I don't know which kind of "terminal" the buildbot and the GitHub Action Windows jobs use.

These days, I run tests in a SSH connection on a Linux terminal to my Windows VM. In this case, apparently, Python seems to be satisfied with the terminal that it gets.

@vstinner
Copy link
Member

vstinner commented Sep 9, 2023

A first step would be to only skip tests which are known to fail, and run the other tests which are fine.

@aisk
Copy link
Member Author

aisk commented Sep 9, 2023

For the test_kbhit, maybe some parallel tests may simulate keyboard hit, so I think maybe we should not check the result value, just test the function exists and not raise exceptions?

@aisk aisk deleted the test-msvcrt branch September 9, 2023 11:38
@terryjreedy
Copy link
Member

I am done with this as these errors are out of my league.

@terryjreedy
Copy link
Member

@zooba knows more and wrote some of the changes.

@aisk
Copy link
Member Author

aisk commented Sep 10, 2023

I figured out what happened:

The test_winconsoleio may have left over contents in the stdin, and if it runs before test_msvcrt, the call of msvcrt.getwch() in test_msvcrt.test_getwch may get a invalid \n result which is written in test_winconsoleio, also the test_kbhit.

One should paste the test case in test_winconsoleio:

def test_ctrl_z(self):
with open('CONIN$', 'rb', buffering=0) as stdin:
source = '\xC4\x1A\r\n'.encode('utf-16-le')
expected = '\xC4'.encode('utf-8')
write_input(stdin, source)
a, b = stdin.read(1), stdin.readall()
self.assertEqual(expected[0:1], a)
self.assertEqual(expected[1:], b)
to the test cases in current test_msvcrt to reproduce the error result.

I tried to call FlushConsoleInputBuffer via ctypes in #109226 to fix the error.

@aisk
Copy link
Member Author

aisk commented Sep 10, 2023

Tests still fails😂Looks like there are more issues...

@vstinner
Copy link
Member

Is it possible to create a "new terminal" just to run test_msvcrt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants