remove type_hint_tests and convert the files to use the new test style#53167
remove type_hint_tests and convert the files to use the new test style#53167guilhermeleobas wants to merge 18 commits intopytorch:masterfrom guilhermeleobas:type_hint_tests
Conversation
💊 CI failures summary and remediationsAs of commit ba7650c (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
|
Overall looks good, thanks @guilhermeleobas. The renamed |
Codecov Report
@@ Coverage Diff @@
## master #53167 +/- ##
=======================================
Coverage 77.34% 77.34%
=======================================
Files 1887 1887
Lines 184826 184826
=======================================
+ Hits 142954 142959 +5
+ Misses 41872 41867 -5 |
|
Hi @rgommers, done! I've deleted |
facebook-github-bot
left a comment
There was a problem hiding this comment.
@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| files = | ||
| torch, | ||
| caffe2, | ||
| test/type_hint_tests, |
There was a problem hiding this comment.
I think I'm missing some context; why is this line being removed?
There was a problem hiding this comment.
That whole directory is being removed. All the files in it test very little, and we now have a much better way of testing with the "reveal" style tests under test/typing/.
There was a problem hiding this comment.
Ah ok thanks! Followup question: how does this relate to #50513? Like I see that now we have two different test files (test/test_type_hints.py and test/test_typing.py) that both run mypy
There was a problem hiding this comment.
Those two files do different things: the first one just runs mypy over the whole code base, the second one has specific unit tests for individual annotations. I'm not sure I see a compelling reason to want to run them in a single command. It's possible of course, but mypy is just a tool and type annotations are just one code aspect - we also don't say we want to run (e.g.) all autograd tests in a single command. There's just tests scattered in places.
The one thing that would be good is better mypy cache reuse. Right now test_type_hints.py can generate caches in the repo root and in test/, and test_typing.py in test/typing/. It'd be nice if all caches for the same .ini file ended up only in the repo root.
There was a problem hiding this comment.
Makes sense :) Regarding cache reuse, though, would that necessarily be better if they all shared the same cache dir? It seems like that might provide opportunity for additional unnecessary cache invalidation, since the commands being invoked are somewhat different from each other.
There was a problem hiding this comment.
True, I'm not quite sure if it would be better. As long as the regression tests only take 10 seconds or so (which is the case now), it's not even worth optimizing I think.
walterddr
left a comment
There was a problem hiding this comment.
lgtm. any additional comments?
|
@walterddr merged this pull request in bbb06c0. |
pytorch#53167) Summary: This is a follow-up PR of pytorch#52408 and move/convert all files under `test/type_hint_tests/*.py` to use the new test style. Pull Request resolved: pytorch#53167 Reviewed By: ejguan Differential Revision: D27081041 Pulled By: walterddr fbshipit-source-id: 56508083800a5e12a7af88d095ca26229f0df358
This is a follow-up PR of #52408 and move/convert all files under
test/type_hint_tests/*.pyto use the new test style.