Require Python file handers to import helpers via root path

[stephenmcgruer]

For web-platform-tests/rfcs#65

Note this will likely require an RFC of its own if we go ahead with it.

[stephenmcgruer]

cc @jgraham and @Hexcles to take a first look and give any early feedback.

I've analysed the results of trigger runs on Chromium Dev for both commit 1 and commit 2, and summarized them here https://docs.google.com/spreadsheets/d/14tocLn8d7y1_CPsKGCyC3HC5KWLmi0_VcMtaksQCn1o/edit?usp=sharing

Overall the results revealed one more location that I had overlooked, which I fixed in commit 3.

Still to do:

• Figuring out whether we should explicitly add _WPT_ROOT to the path here, or if its ok to rely on the fact that it seems to already be there (?!).
• Fixing the tests.
• Documentation updates for the new world.

[stephenmcgruer]

Well, this is fun. Adding an __init__.py entry in the root of the project (321c418) breaks a bunch of unittests that can no longer resolve from tools ... imports, because suddenly WPT's root is no longer on the path. I don't know why it isn't on the path anymore or why it is on the path normally for the unittests.

[Hexcles]

I can answer why it works currently.

There are two cases, both of which involve the CWD.

• Some non-test code can import tools because its code path starts from ./wpt and you have tools in CWD.
• Some tests can import tools because tools/tox.ini says changedir = {toxinidir}/...

[stephenmcgruer]

Some tests can import tools because tools/tox.ini says changedir = {toxinidir}/...

This isn't true for py27 in tools/tox.ini though, is it? changedir appears to only apply to the mypy cases?

[stephenmcgruer]

Ah, it's a general pytest feature. (Maybe. If I'm reading this right).

https://docs.pytest.org/en/stable/pythonpath.html#prepend-and-append-import-modes-scenarios

pytest will find foo/bar/tests/test_foo.py and realize it is part of a package given that there’s an init.py file in the same folder. It will then search upwards until it can find the last folder which still contains an init.py file in order to find the package root (in this case foo/). To load the module, it will insert root/ to the front of sys.path (if not there already) in order to load test_foo.py as the module foo.bar.tests.test_foo.

[Hexcles]

Ahh sorry for adding to the confusion. You're right, and this also explains why your change breaks it. With the addition of root-level __init__.py, pytest now considers the repo root as the package root so the import path needs to be wpt.tools instead of tools.

[stephenmcgruer]

So this is, I think, the set of changes needed to make the unit tests pass. (Best reviewed by commit, I think). It's not pretty, but PTAL.

There's one failure in Windows that I think is unrelated, but not sure.

[stephenmcgruer]

RFC: web-platform-tests/rfcs#68

[Hexcles]

LGTM

[jgraham]

Do we have a way to enforce the lack of local imports? We could do something like

old_modules = set(sys.modules.keys())
[...]
for name, module in sys.modules.items():
  if name not in old_modules:
    path = os.path.dirname(module.__file__)
    if path.startswith(self.docroot):
      mod_name = os.path.relpath(path, self.docroot).replace(os.path.sep, ".")
      if mod_name != name:
        raise HTTPError("Illegal import in handler")

although it's not lovely.

[stephenmcgruer]

Do we have a way to enforce the lack of local imports? We could do something like

I think I would rather lint it than do something nasty to sys.modules again. I think a lint looking at only .py files outside of e.g. tools would be fairly cheap and mostly reliable. If we're worried about hitting e.g. generator scripts and other non-handlers, we could also look for def main(request, response) (again not perfect given you can technically call the parameters whatever you want, but I would guess that 99%+ of handlers use the default names there).

[stephenmcgruer]

RFC has now merged. I've done another comparison study on Chrome dev with/without this PR: https://docs.google.com/spreadsheets/d/14tocLn8d7y1_CPsKGCyC3HC5KWLmi0_VcMtaksQCn1o/edit#gid=1578637675

Only concerning result is resource-timing/resource-timing-level1.sub.html, but I can't reproduce the failure locally with the patch. Will keep an eye on it post-merge to see if we've accidentally broken something here.