-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Update multiple unittests from CPython 3.10.5 #3910
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
Update multiple unittests from CPython 3.10.5 #3910
Conversation
b8fa3e2
to
ebc7b57
Compare
Lib/test/test_array.py
Outdated
def test_imul(self): | ||
super().test_imul() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the ones around here that no longer have the @unittest.expectedFailure
decorators on them be removed? @frank-king left these behind without realizing why I put them there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fanninpm you don't need to mention someone unless we actually need their attention. In this case, I think referring commit or pr url can be helpful, but not about the author.
Lib/test/test_deque.py
Outdated
# TODO: RUSTPYTHON | ||
@unittest.expectedFailure | ||
def test_contains_fake(self): | ||
super().test_contains_fake() | ||
|
||
# TODO: RUSTPYTHON | ||
@unittest.expectedFailure | ||
def test_count(self): | ||
super().test_count() | ||
|
||
# TODO: RUSTPYTHON | ||
@unittest.expectedFailure | ||
def test_index(self): | ||
super().test_index() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I add things like this, I'd like to direct contributors against leaving the last two lines of this pattern behind when they un-mark these tests.
Lib/test/test_list.py
Outdated
# TODO: RUSTPYTHON | ||
@unittest.expectedFailure | ||
def test_count(self): | ||
super().test_count() | ||
|
||
# TODO: RUSTPYTHON | ||
@unittest.expectedFailure | ||
def test_contains_fake(self): | ||
super().test_contains_fake() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I add things like this, I'd like to direct contributors against leaving the last two lines of this pattern behind when they un-mark these tests.
Lib/test/test_userlist.py
Outdated
# TODO: RUSTPYTHON | ||
@unittest.expectedFailure | ||
def test_contains_fake(self): | ||
super().test_contains_fake() | ||
|
||
# TODO: RUSTPYTHON | ||
@unittest.expectedFailure | ||
def test_count(self): | ||
super().test_count() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I add things like this, I'd like to direct contributors against leaving the last two lines of this pattern behind when they un-mark these tests.
ebc7b57
to
a5dac57
Compare
a5dac57
to
6842f9b
Compare
@fanninpm I added additional comments for new methods from rustpython and added 4 more commits |
6842f9b
to
6729417
Compare
# TODO: RUSTPYTHON | ||
@unittest.expectedFailure | ||
def test_index(self): | ||
super().test_index() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, classes dependent on this test may start unexpectedly passing. Will they unexpectedly pass all at once or one at a time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are supportive code. They will be handled when they actually be solved. We can spend cost when we actually need it.
The answer is I don't know. Sometimes they passes at once, sometimes not. It depends on problems and solutions. I didn't look in the details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor cleanup notes.
def test_init(self): | ||
super().test_init() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test_init
method in list_tests.CommonTest
is no longer marked. I don't think this needs to be here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fanninpm we expect code review is about changed code, instead of arbitrary code around changed code.
I think that change is independent from this PR. If you want me to fix it, I will do it in separated PR.
@unittest.expectedFailure | ||
def test_count(self): # XXX: RUSTPYTHON; the method also need to be removed when done | ||
super().test_count() | ||
|
||
# TODO: RUSTPYTHON, parent method is marked but this version passes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test_repr_deep
method in list_tests.CommonTest
is no longer marked, so this comment is currently outdated.
No description provided.