Require Python file handers to import helpers via root path by stephenmcgruer · Pull Request #26328 · web-platform-tests/wpt

stephenmcgruer · 2020-10-29T16:35:12Z

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 · 2020-11-04T19:28:52Z

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 · 2020-11-04T20:26:05Z

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 · 2020-11-04T20:56:40Z

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 · 2020-11-04T20:59:09Z

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 · 2020-11-04T21:04:10Z

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 · 2020-11-09T20:21:00Z

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.

Implementation CL: web-platform-tests/wpt#26328

stephenmcgruer · 2020-11-10T16:11:07Z

RFC: web-platform-tests/rfcs#68

Hexcles

LGTM

tools/ci/tc/tests/test_decision.py

tools/wptserve/wptserve/router.py

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 · 2020-11-13T14:56:04Z

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).

Implementation CL: web-platform-tests/wpt#26328

stephenmcgruer · 2020-11-17T15:29:40Z

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.

…26328)" This reverts commit b595c93.

wpt-pr-bot had a problem deploying to wpt-preview-26328 October 29, 2020 16:41 Error

wpt-pr-bot temporarily deployed to wpt-preview-26328 October 29, 2020 18:28 Inactive

stephenmcgruer force-pushed the smcgruer/py_file_handlers branch from 58e0127 to b6faaff Compare November 2, 2020 19:10

wpt-pr-bot requested a deployment to wpt-preview-26328 November 2, 2020 19:15 Pending

stephenmcgruer mentioned this pull request Nov 3, 2020

RFC 65: Switching to "Py3-first" for WPT tests on CI web-platform-tests/rfcs#65

Merged

stephenmcgruer commented Nov 3, 2020

View reviewed changes

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 3, 2020 16:41 Inactive

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 4, 2020 14:35 Inactive

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 4, 2020 16:41 Inactive

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 4, 2020 16:51 Inactive

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 4, 2020 17:18 Inactive

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 4, 2020 17:35 Inactive

stephenmcgruer force-pushed the smcgruer/py_file_handlers branch from 278f881 to 321c418 Compare November 4, 2020 19:21

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 4, 2020 19:25 Inactive

stephenmcgruer changed the title ~~[WIP] Remove automated path setting and sys.module copying for py fil…~~ Require Python file handers to import helpers via root path Nov 4, 2020

stephenmcgruer marked this pull request as ready for review November 4, 2020 21:13

wpt-pr-bot added assets cookies docs infra resource-timing wg-webperf wptserve labels Nov 4, 2020

wpt-pr-bot assigned stephenmcgruer Nov 4, 2020

wpt-pr-bot requested a review from deniak November 4, 2020 21:14

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 9, 2020 18:51 Inactive

wpt-pr-bot added the wpt label Nov 9, 2020

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 9, 2020 19:17 Inactive

wpt-pr-bot added lint manifest labels Nov 9, 2020

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 9, 2020 19:59 Inactive

stephenmcgruer requested review from Hexcles and jgraham November 9, 2020 20:19

stephenmcgruer added a commit to web-platform-tests/rfcs that referenced this pull request Nov 10, 2020

RFC ??: Make local imports in Python file handlers root-relative

9c8df1b

Implementation CL: web-platform-tests/wpt#26328

stephenmcgruer mentioned this pull request Nov 10, 2020

RFC 68: Make local imports in Python file handlers root-relative web-platform-tests/rfcs#68

Merged

Hexcles approved these changes Nov 10, 2020

View reviewed changes

tools/ci/tc/tests/test_decision.py Show resolved Hide resolved

tools/wptserve/wptserve/router.py Show resolved Hide resolved

wpt-pr-bot temporarily deployed to wpt-preview-26328 November 11, 2020 18:43 Inactive

jgraham approved these changes Nov 12, 2020

View reviewed changes

stephenmcgruer added 3 commits November 17, 2020 08:17

Require Python file handers to import helpers via root path

3c74416

Add docs for local imports for file handlers, first pass

4d85e03

Fix wptserve unittests

b4e2607

stephenmcgruer force-pushed the smcgruer/py_file_handlers branch from bc59dba to b4e2607 Compare November 17, 2020 13:18

wpt-pr-bot deployed to wpt-preview-26328 November 17, 2020 13:20 View deployment

stephenmcgruer added a commit to web-platform-tests/rfcs that referenced this pull request Nov 17, 2020

RFC 68: Make local imports in Python file handlers root-relative (#68)

ba0fece

Implementation CL: web-platform-tests/wpt#26328

stephenmcgruer merged commit b595c93 into master Nov 17, 2020

stephenmcgruer deleted the smcgruer/py_file_handlers branch November 17, 2020 15:32

moz-wptsync-bot added the mozilla:gecko-blocked label Nov 17, 2020

stephenmcgruer mentioned this pull request Nov 17, 2020

Cleanup unnecessary __init__.py files once Py3-only #26552

Closed

moz-wptsync-bot removed the mozilla:gecko-blocked label Nov 18, 2020

stephenmcgruer added a commit that referenced this pull request Nov 18, 2020

Revert "Require Python file handers to import helpers via root path (#…

c3745bc

…26328)" This reverts commit b595c93.

stephenmcgruer mentioned this pull request Dec 2, 2020

[Python3] encoding import error with threaded calls to Python file handlers #25678

Closed

Conversation

stephenmcgruer commented Oct 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephenmcgruer left a comment

Choose a reason for hiding this comment

Uh oh!

stephenmcgruer commented Nov 4, 2020

Uh oh!

Hexcles commented Nov 4, 2020

Uh oh!

stephenmcgruer commented Nov 4, 2020

Uh oh!

stephenmcgruer commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hexcles commented Nov 4, 2020

Uh oh!

stephenmcgruer commented Nov 9, 2020

Uh oh!

stephenmcgruer commented Nov 10, 2020

Uh oh!

Hexcles left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jgraham left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephenmcgruer commented Nov 13, 2020

Uh oh!

stephenmcgruer commented Nov 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stephenmcgruer commented Oct 29, 2020 •

edited

Loading

stephenmcgruer commented Nov 4, 2020 •

edited

Loading

jgraham left a comment •

edited

Loading