testcheck: Automated detection of .test files #8650 #11453
Conversation
|
Actually you don't have to open a new pull request. Pushing to the same branch is simpler and can help tracking the discussion history. |
Yaa, I am a little new to the concept of pull requests, sorry. Are there any more changes required in this commit? |
|
I understood what the error was, it was a self mypy type check error. I solved 3 of the conflicts by simply converting the strings to pathlib Path datatype. But using the Path datatype variable is now posing problems in other parts of the code that maybe don't use the Path function from pathlib. Some insights from the contributors would be really appreciated, thanks. ''' |
|
Dear Contributors, I separately cloned the original repository to another location and tried to type check testcheck.py, and to my surprise, the above mentioned mypy generated typecheck errors still persisted. Perhaps these typecheck errors are not caused by my changes committed, or if it is, can anyone please guide me as to how I should proceed next? Also, sorry for the fuss created with the new pull requests and all that, I am still learning. Thanks! |
mypy/test/testcheck.py
Outdated
| typecheck_files = [] | ||
| for dirpath, dirnames, filenames in os.walk(test_path): | ||
| for filename in [f for f in filenames if f.endswith(".test") and f.startswith("check-")]: | ||
| typecheck_files.append(filename) |
There was a problem hiding this comment.
I'm pretty sure this entire thing could be improved by using a .rglob call and a single list comprehension with an if statement
mypy/test/testcheck.py
Outdated
| test_path = Path(test_path).parents[1] | ||
| test_path = test_path / Path('test-data') | ||
| test_path = test_path / Path('unit') |
There was a problem hiding this comment.
- First thing here accessing current working directory.parents and then the 2nd item in that seems incredibly unreliable as if someone doesn't have this in a specific level on their machine this will fail.
- Secondly
Path.__truediv__supports using strings directly instead of manually creating an instance ofPath. - Thirdly assigning to test_path here multiple times doesn't make much sense you should just put this all on one line like:
test_path = Path.cwd().parent.parent / 'test-data' / 'unit'There was a problem hiding this comment.
Ok I will make the necessary changes.
Also, will a user have different paths to the test files? in that case, can I simply set the test path to the root directory, so that all test files starting with 'check-' are detected, regardless of where they are stored?
There was a problem hiding this comment.
No if someone has moved the tests from the mypy root that's their fault and shouldn't be something you can account for
There was a problem hiding this comment.
Ok, noted.
I have committed the necessary changes, please do review them @Gobot1234
Thanks for the cooperation!
There was a problem hiding this comment.
It's looking much better, good job! Last thing would be making sure that the version conditional tests for 3.8 and 3.9 are correctly appended (currently they are both going to be there even if the features are unsupported as far as I can tell)
Co-authored-by: James Hilton-Balfe <50501825+Gobot1234@users.noreply.github.com>
|
Just made the change for separate testing of python 3.8 and 3.9. For now, it has been hard-coded, please let me know if you were expecting a more refined approach, Thanks! |
|
The code seems to have failed in the flake8 test case. Can anyone explain me the error I need to tackling in this case? @Gobot1234 |
mypy/test/testcheck.py
Outdated
| 'check-slots.test', | ||
| 'check-formatting.test' | ||
| ] | ||
| typecheck_files = [path.name for path in (Path.cwd().parent.parent / 'test-data' / 'unit').rglob('check-*.test') if path.name != 'check-python38.test' and path.name != 'check-python39.test'] |
There was a problem hiding this comment.
The error you are getting is by a code linter, in this case it is saying this line is too long.
I would recommend rewriting this list comprehension into a for loop, as that will be more readable anyway.
There was a problem hiding this comment.
Yaa sure, I'll get on it.
I had a different doubt:
I have seen another issue that I can work on, but I am confused as to how to make two parallel PRs without one's changes being reflected on the others. I have forked the mypy repo, should I fork it again? I am really confused as I am new to this. Some guidance will be really grateful, thanks!
There was a problem hiding this comment.
For sure! So in git, you can switch to a different branch. I might recommend reading over this guide, which covers the basics, as well as learning a bit more about git in general.
There was a problem hiding this comment.
Ok, thanks!
I have committed the necessary changes, you can run the workflow tests on the code now, I guess.
|
It seems that some files aren't loaded. The number of total tests should be 9000~ |
I don't understand. Were there 9000 test files in the initial typecheck_files list? Or is this code hampering other test file detection? |
SwagatSBhuyan
changed the title
There was a problem hiding this comment.
We have around 9000 test cases that are executed during test phase. All these test cases are located in different test files.
Right now, this branch discovers around 4100 test cases:
=========== 4193 passed, 354 skipped, 2 xfailed in 667.35s (0:11:07) ===========
Source: https://github.com/python/mypy/runs/4129333578?check_suite_focus=true#step:8:82
But, here's what happens on master right now:
=========== 9588 passed, 370 skipped, 7 xfailed in 783.78s (0:13:03) ===========
Source: https://github.com/python/mypy/runs/4203209268?check_suite_focus=true#step:8:157
Looks like not all files are loaded, that's why we have less test cases in total.
All I did was find a way to add the already mentioned test files into the typecheck_files py script. And the test files detected before and after the commits were also same. The only difference is I excluded the python38 and python39 test files as they were tested for in the subsequent code. If you could maybe troubleshoot this problem along with me that'd be great! |
I just did a little bit of trouble shooting and I found out that after the implementation of the new commits, P.S.: I just noticed that just like python38 and python 39 test cases not being added to the list, |
This doesn't sound right. I managed to reproduce the problem and there were a lot of differences in between the tests found with and without this PR than the tests in check-modules-case.test. This makes sense, because there are only 11 tests in check-modules-case.test, which is far fewer than the around 5000 missing tests caused by this PR. Side note: For me, the tests in check-modules-case.test were actually included in both the new and old versions, but I'm guessing that depends on what platform you're on. Are you sure you're actually reproducing the bug correctly? When you try to reproduce this, are you getting around 4000 tests collected like in the test runs on this PR or are you getting around 9500 tests like in the test runs on master? If the only difference you're seeing is check-modules-case.test, it sounds like you're getting around 9500 tests. If you aren't able to reproduce the bug locally, you could also try debugging on CI and seeing what the CI run is finding, although that is admittedly more annoying.
I agree that this should be fixed, but I don't think that it's the cause of the drop in number of tests found. As you said, this is getting included when it previously wasn't, so if anything, it should increase the number of tests found, not decrease it. |
Fixed: issue #8650
On merging this PR, the testcheck.py script will automatically walk the test file directory and record all the test files starting with 'check-'. This is very crucial in order for this problem to not emerge in future endeavors when new test files are added into the repository, as the file searching is automated to detect any new such files.
This approach is not using regex, rather uses simple python os library methods.
The changes have been verified on my local system, the python script after changes runs smoothly. The newly detected test files are shown in this helper local python script terminal (an additional 4 files are detected from a subdirectory):
On suggestions from the contributors, I have used the pathlib library to make it work across various platforms. Also, walking is enabled so that 'check-' test files from deeper subdirectories can also be detected.
It would be great if this PR is authorized, I could henceforth start my journey to contributing to mypy after this.
Thanks @JukkaL @ethanhs @davidzwa