gh-127146: Update test skips for Emscripten 4.0.2 #129474
Conversation
| @@ -502,6 +499,8 @@ class UnseekableWriter(self.MockUnseekableIO): | |||
| (self.BytesIO, "rws"), (self.StringIO, "rws"), | |||
| ) | |||
| for [test, abilities] in tests: | |||
| if test == pipe_writer and not threading_helper.can_start_thread: | |||
| continue | |||
There was a problem hiding this comment.
Should this use skipTest rather than continue?
There was a problem hiding this comment.
If we wanted to do that we'd have to split the with statement below like:
with self.subTest(test):
if cond:
skipTest()
with test() as obj:
...
and then we'd have to indent the whole body. I think it's better to use continue than to have to indent a bunch of stuff another level.
There was a problem hiding this comment.
Agree that it requires some code churn (and some deeper-than-ideal nesting).
My motivation here is to make sure that we have visibility on the skip - with a continue, there's no tracking that the test wasn't executed. If the code ever skips for the wrong reason, or you need to audit to see what isn't being tested, there's no visibility on that.
How about a third option: Pull out the current test logic as a _subtest() utility method that takes test as an argument. That way, the indent level for the body of the test stays the same; the only code churn is to move the for loop to after the subtest utility method. However, that for loop becomes a much clearer "run this subtest for each configuration" loop, with clear "run or skip" logic for each case.
There was a problem hiding this comment.
I think this makes sense to me I'll try it out.
There was a problem hiding this comment.
Though if we're skipping because of threading I guess the only question is whether the feature in question could be tested without threading. Other than that there's nothing here to fix.
There was a problem hiding this comment.
Agreed that's another way to fix this... but I don't see an obvious way to implement the pipe writer test without threading (at least, not without leaving the test prone to blocking). I'm open to suggestions, though.
|
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 |
Emscripten 4.0.2 brought in a few new file system bug fixes.