Repository navigation
feat(test): mypyc tests for container creation from range - #19511
BobTheBuidler wants to merge 11 commits into
Conversation
BobTheBuidler
changed the title
rangerange
eb429b3 to
76c46c8
Compare
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
The volume of these tests seems too much -- they seem to add about 5% to the total line count of irbuild tests, and the use case isn't very common.
We don't normally use irbuild tests to check lots of edge cases/variants -- we use them to make sure the quality of code for common or tricky use cases doesn't regress. When IR is changed, irbuild tests often need changes as well, and if too many irbuild tests need to be changed, reviewers are not likely to pay much attention to them and regressions are more likely to go unnoticed.
However, adding many of these as run tests would be reasonable, since they don't require much maintenance, and if run tests are added as additional def test_... cases to an existing test case, they won't slow down tests much. Top-level run tests indicated by [case ...] are quite slow and thus we try to group multiple related tests together, but adding additional cases to an existing [case ...] doesn't cause much slowdown.
|
|
||
| [case testTupleFromRange1] | ||
| def fn() -> tuple[int, ...]: | ||
| return tuple(range(3)) |
There was a problem hiding this comment.
Similar to above, tuple(range(...)) seems very rare (based on a large codebase I have access to -- there were no instance of it). Again, having run tests seems sufficient since irbuild tests are verbose and take some maintenance, preferable added to an existing run test.
If you create a PR that optimizes this, I wouldn't spend extra effort on tuple specifically, but if an optimization also benefits tuples, it would be enough to have some example IR dumps for relevant code in the PR summary.
|
|
||
| [case testListFromRange1] | ||
| def fn() -> list[int]: | ||
| return list(range(3)) |
There was a problem hiding this comment.
list(range(...)) seems to be by far the most common of these use cases, so having some irbuild tests for them seems reasonable.
| [case testListFromRange4] | ||
| def fn() -> list[str]: | ||
| abc = tuple(range(3)) | ||
| return list(str(i) for i in abc) |
There was a problem hiding this comment.
This feels too specific for an irbuild test -- again, can you move this to be a run test?
| [case testListFromRange5] | ||
| def fn() -> list[str]: | ||
| abc = tuple(range(1, 3)) | ||
| return list(str(i) for i in abc) |
There was a problem hiding this comment.
Another test that feels pretty specific. Do you have some real-world use case in mind?
| [case testListFromRange6] | ||
| def fn() -> list[str]: | ||
| abc = tuple(range(1, 3, 2)) | ||
| return list(str(i) for i in abc) |
| return 1 | ||
|
|
||
| [case testSetFromRange1] | ||
| def fn() -> set[int]: |
There was a problem hiding this comment.
I was able to find some example of set(range(...)), but only a few. I think the first two irbuild test cases are sufficient, due to maintainability concerns. These would be reasonable as run tests (preferable as new cases in an existing test case).
|
In the past we've deleted some irbuild tests, since their maintenance took surprisingly lot of effort, and that's why I think it's better to have most things tested using run tests. Performance issues can also be tested using benchmarks (https://github.com/mypyc/mypyc-benchmarks), but they are also quite a heavy tool so they only work for the most common cases. |
de9114f to
347c8bb
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
347c8bb to
7aca7ce
Compare
2 participants
This PR only adds tests to the existing suite.
I will be using them to ensure the quality of a future PR.