Clean up beyond_top_level tests #7279
Conversation
Pull Request Test Coverage Report for Build 2849766793
💛 - Coveralls |
| # The first package fails to lint | ||
| assert len(output.split("\n")) == 1 |
There was a problem hiding this comment.
Should we leave this as 5 and xfail it? Tests are documentation, and I don't want to document the wrong result.
There was a problem hiding this comment.
Well, maybe the result is correct if the expectation is that you have to cd to a namespace package you want to lint.
There was a problem hiding this comment.
Yeah I was meaning to ask this. Is the new behavior how we want to do this? Or is this a temporary result because of improved but not perfect namespace package handling?
If this is desired (ie., you can't lint a package like this) I think it makes sense to just have this as a test itself.
There was a problem hiding this comment.
Thanks for giving me time to double-check. Yes, you need to cd or otherwise do something fancy (with a hook?) if you want to find namespace packages. This matches the behavior of python's import statement, so far as I can tell. So the result is correct.
To go back to the original issue linked in these tests, we're just trying to test for the absence of no-name-in-module or relative-beyond-top-level.
There was a problem hiding this comment.
I was wrong about this after all. I guess the beyond_top_two is on sys.path somewhere, so it should be linted. (I guess because tests is on sys.path?) See #7304.
Ultimately new tests should create their own isolated packages with code, to make this easier to reason about!
Type of Changes
Description
Closes #7252.