Closed Bug 1823701 Opened 2 years ago Closed 2 years ago

Running web-platform tests fail since ruff is active with error: "No module named 'tomli'"

Categories

(Developer Infrastructure :: Lint and Formatting, defect)

defect

Tracking

(firefox-esr102 unaffected, firefox111 unaffected, firefox112 unaffected, firefox113 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr102 --- unaffected
firefox111 --- unaffected
firefox112 --- unaffected
firefox113 --- fixed

People

(Reporter: whimboo, Assigned: ahal)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Since the patch on bug 1811850 landed I'm not able to run any web-platform tests like the ones for WebDriver. The following command fails for me:

$ mach wpt --webdriver-binary=target/debug/geckodriver  testing/web-platform/tests/webdriver/tests/bidi/browsing_context/create/type.py
[..]
 0:19.98 INFO Starting runner
 0:20.37 TEST_START: /webdriver/tests/bidi/browsing_context/create/type.py
 0:20.54 TEST_END: ERROR, expected OK - No module named 'tomli'
 0:20.54 INFO No more tests

Note that none of the tests directly try to import tomli, so I don't know where this is coming from and why tests in CI are working fine. I tried a clobber but that didn't help.

Andrew, can you please check that? Thanks!

Flags: needinfo?(ahal)

Please drop the CI part. In Taskcluster we do not use mach but mozharness.

So I haven't reproduced yet, but looks like this is caused by pytest:
https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/third_party/pytest/src/_pytest/config/findpaths.py#67

I'd guess that by adding the pyproject.toml file, it triggers this code path which in turn needs tomli? So I think we have two options:

  1. Add tomli to the requirements for mach wpt
  2. We could rename pyproject.toml to .ruff.toml (though I was kind of hoping to use pyproject.toml to avoid proliferation of top-level config files)

Any preference? I'll do some testing in a bit here.

I can confirm that adding tomli to:
https://searchfox.org/mozilla-central/source/python/sites/wpt.txt

Fixes the issue for me. I'd suggest we just do this because I'd prefer to use a single standard config file for all our Python stuff rather than a bespoke .ruff.toml file.

Flags: needinfo?(ahal)

So in general I don't love adding new dependencies for wpt, but I guess in this case it would only be required in m-c, not upstream. So if we can configure it purely in Mozilla code, I don't have a strong preference either way (but I wonder if we have other pytest-using harnesses that might be affected?)

Note I'm not sure why tomli isn't vendored as a pytest dependency. If I pip install pytest it pulls in tomli==2.0.1 as expected.. So possibly figuring out why that's the case is the real proper fix here.

Yeah, so for other pytest harnesses, it looks like tomli is pulled in as a pytest dependency. Maybe WPT is using an older version of pytest that has a bug in its dependencies? Or possibly it used to be an "extra" but since moved to the main set?

I'm happy to just use the wpt.txt fix to get us unblocked for now though. I'll attach a patch.

Since we added a root pyproject.toml file, it triggered a code path in pytest
which tries to open the file to read configuration with tomli. For whatever
reason, this isn't vendored for wpt and we therefore get import errors.

Assignee: nobody → ahal
Status: NEW → ASSIGNED

Hm, the pytest package requires it here:

https://searchfox.org/mozilla-central/rev/f078cd02746b29652c134b144f0629d47e378166/testing/web-platform/tests/tools/third_party/pytest/setup.cfg#50

Maybe with some former pytest update we missed to add it to the patch?

Python vendoring in upstream wpt is… not great. It's very possible that we added pytest in a way that didn't automatically grab all the dependencies and relied on runtime tests to ensure that things still work. Which in this case they would until you add an unrelated file into the repo, at which point things fall apart.

Pushed by ahalberstadt@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b659224e6ab4 Add 'tomli' to mach wpt requirements, r=jgraham
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: