[2.7] bpo-30283: Backport test_regrtest from master to 2.7#1513
Conversation
|
Ah, as expected the test failed on Windows. I made a first serie of changes to be able to the error messages :-) |
Function used by test_regrtest to simulate an interrupted unit test.
There was a problem hiding this comment.
testdir is not passed to runtest() in 3.5.
Do you want to port this to 3.5?
There was a problem hiding this comment.
testdir is not passed to a subprocess in 3.x.
Do you want to port this to 3.5-3.7?
There was a problem hiding this comment.
Oh. I expected that only 2.7 has an old regrtest, but 3.5 also has the "old" regrtest. This change is required by test_regrtest for tests using -jN. It seems like we don't have these tests in 3.5 yet: test_regrtest only has unit tests on the command line parsing. Said differently, Python 3.5 is also impacted by bpo-30283. I added a comment in the issue.
There was a problem hiding this comment.
But master don't pass testdir too.
There was a problem hiding this comment.
But master don't pass testdir too.
In master, there is no need to pass testdir since regrtest is designed differently. run_test_in_subprocess() pass "ns" a dict which contains testdir. In the slave, setup_tests() is called with ns and so sys.path is updated properly.
I hate regrtest.py of Python 2.7/3.5, I would prefer to just replace it with master libregrtest...
There was a problem hiding this comment.
Ah, it is passed in slaveargs!
There was a problem hiding this comment.
testdir is not passed to runtest() in 3.5.
There was a problem hiding this comment.
'returncode stdout, stderr' looks strange.
This namedtuple is used only in two places, and only its stdout attribute is used. Wouldn't be simpler to return just stdout from run_command()? Or at least return a 3-tuple?
There was a problem hiding this comment.
I prefer to mimick the API of the master branch to ease backports of more tests later.
There was a problem hiding this comment.
In any case using different delimiters looks strange.
There was a problem hiding this comment.
Oh wow, I didn't notice that I did two mitakes: typo in the type name, and "," typo in the string describing fields. I'm surprised because "," seems to be simply ignored!? Fixed in PR 1516.
>>> collections.namedtuple('Point', 'x,y z')._fields
('x', 'y', 'z')
It seems like namedtuple tries to be kind and fix my mistakes :-)
There was a problem hiding this comment.
Do you want to port this test to 3.5?
There was a problem hiding this comment.
PermissionError doesn't exist in 2.7.
There was a problem hiding this comment.
Oh, nice catch. Too bad that we don't have basic flake8 tests on Python...
There was a problem hiding this comment.
Why not list comprehension?
There was a problem hiding this comment.
The code comes from master, I don't remember why I used that syntax. Please modify directly the code in the master branch if you don't like it :-)
There was a problem hiding this comment.
Wouldn't be better to compare sorted lists? This would check that tests are not duplicated.
In 3.x you can use assertCountEqual.
There was a problem hiding this comment.
Again, the code is copied from the master branch. Any enhancement is welcome, but should be made first in the master branch ;-) I like your idea of comparison of sorted lists. I don't know assertCountEqual().
There was a problem hiding this comment.
Why executed and tests are added in the third argument?
There was a problem hiding this comment.
When a test failed, I didn't see executed and tests values. Maybe I just missed them in the output, but it was quicker to use this tuple to fix my bug.
There was a problem hiding this comment.
Is it worth to add '-3'?
There was a problem hiding this comment.
Since "make buildbottest" uses it: ok, I will add it in my following change.
|
Oh, you again merged while I made a review. |
|
And again just 2 minutes before I finished it. 😄 |
Did you loose your review? You can still review the change, no? |
|
No, my review isn't lost. You can read it. |
3 participants
No description provided.