GH-100479: Fix pathlib test failure on WASI #104215
Conversation
barneygale
commented
May 5, 2023
•
barneygale
commented
- Issue: Support for sharing state between pathlib subclasses #100479
| @@ -1655,7 +1655,8 @@ class P(_BasePurePathSubclass, self.cls): | |||
| p = P(BASE, session_id=42) | |||
| self.assertEqual(42, p.absolute().session_id) | |||
| self.assertEqual(42, p.resolve().session_id) | |||
| self.assertEqual(42, p.with_segments('~').expanduser().session_id) | |||
| if not is_wasi: # WASI has no user accounts. | |||
There was a problem hiding this comment.
FWIW, I know very little about this module or how the tests are usually done. That said, it looks like all the other uses of is_wasi in this file relate to skipping the whole test, rather than a part of it. Personally, I have no relevant opinion on if your change is okay. 😄 (My irrelevant opinion is that, generally, the behavior of a test should not change based on some environmental condition.)
(Also, FYI, I pointed out the failing buildbot earlier because the first failure happened when a change of mine was merged. That's when I noticed it was probably the pathlib change. Otherwise I don't have any particular interest and probably would not have noticed. 😄)
There was a problem hiding this comment.
I agree with Eric -- if it's just a specific part of this test that fails on WASI, best to separate it out into a separate test method that's decorated with @skipIf(is_wasi, "WASI has no user accounts")
There was a problem hiding this comment.
I agree in principle, but this style of tweaking test behaviour slightly based on what the system supports is pervasive throughout test_pathlib.py. For example, a few lines below we have:
if os_helper.can_symlink():
self.assertEqual(42, (p / 'linkA').readlink().session_id)Is there something to be said for using a consistent approach, even if it's imperfect?
(I don't feel strongly about this, just thought I'd point it out!)
There was a problem hiding this comment.
Hmm. I don't like it, and I'd love to see a PR cleaning all those up ;)
But I guess consistency wins the day for now!
|
!buildbot .wasm. |
|
🤖 New build scheduled with the buildbot fleet by @AlexWaygood for commit 3c74650 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
