Allow patial update to wpt manifest #47588

WeizhongX · 2024-08-13T17:25:41Z

To run WPTs locally, developers usually only run a few of tests each time. Updating WPT manifest for the whole WPT Repo is not necessary and took ~3 seconds on a performant develop machine. And this can take forever if the developer is using a remote file system.

This change allows updating WPT manifest for tests only intended to be run. This brings down the test time for a few tests to ~0.5 seconds.

With this patch applied, we did not see a time increase when updating manifest for the entire repo.

Additional work is needed to plug in this into the test runner.

WeizhongX · 2024-11-05T21:48:56Z

Folks, can you take a look?

Tests are running fine in chromium (https://chromium-review.googlesource.com/c/chromium/src/+/5991965).

jonathan-j-lee

LGTM. I think this lines up with @jgraham's feedback in #47350.

jonathan-j-lee · 2024-11-05T22:00:43Z

tools/manifest/update.py

@@ -79,6 +81,9 @@ def create_parser() -> argparse.ArgumentParser:
    parser.add_argument(
        "--no-parallel", dest="parallel", action="store_false", default=True,
        help="Do not parallelize building the manifest")
+    parser.add_argument('tests',
+                        nargs='*',
+                        help='Paths of test files or directories to update.')


Suggested change

help='Paths of test files or directories to update.')

help=('Test files or directories to update. '

'Omit to update all items under the test root.'))

jonathan-j-lee · 2024-11-05T22:05:46Z

tools/manifest/vcs.py

+                    for filename, path_stat in filenames:
+                        path = os.path.join(subdir,
+                                            os.path.join(dirpath, filename).decode("utf8"))
+                        if mtime_cache is None or mtime_cache.updated(path, path_stat):


nit: Put the shared file case in a internal helper (e.g., yield self._make_file_info(path, path_stat))?

jgraham

I think this needs some tests, and some profiling to indicate no perf regression in the case where we don't pass in any paths.

tools/manifest/vcs.py

jgraham · 2024-11-07T14:07:31Z

tools/manifest/update.py

@@ -79,6 +81,9 @@ def create_parser() -> argparse.ArgumentParser:
    parser.add_argument(
        "--no-parallel", dest="parallel", action="store_false", default=True,
        help="Do not parallelize building the manifest")
+    parser.add_argument('tests',


Presumably I'm missing something, but I don't see where this command line argument is being used.

As I said in the description, Additional work is needed to plug in this into the test runner. This change only make patial update in wpt manifest command possible.

Presumably I'm missing something, but I don't see where this command line argument is being used.

Line 49, above:

wpt/tools/manifest/update.py

Line 49 in b4aa684

paths_to_update=kwargs['tests'],

WeizhongX · 2024-11-07T17:06:15Z

I think this needs some tests, and some profiling to indicate no perf regression in the case where we don't pass in any paths.

Is there a reason why this could cause perf regression? I can get the data though.

WeizhongX · 2024-11-14T01:57:48Z

I tried to run time ./wpt manifest -v --no-download --tests-root=. --url-base=/ with and without this change, and get results as below:

before:
real 0m9.799s
user 1m36.979s
sys 0m13.197s

After:
real 0m9.919s
user 1m36.533s
sys 0m13.205s

So real time increased about 0.12s. If this is consistent, this probably is due to the instroduce of _make_file_info, which is called inside the loop, but guess this is OK?

I think this needs some tests, and some profiling to indicate no perf regression in the case where we don't pass in any paths.

Is there a reason why this could cause perf regression? I can get the data though.

gsnedders · 2024-12-23T21:45:09Z

tools/manifest/update.py

@@ -79,6 +81,9 @@ def create_parser() -> argparse.ArgumentParser:
    parser.add_argument(
        "--no-parallel", dest="parallel", action="store_false", default=True,
        help="Do not parallelize building the manifest")
+    parser.add_argument('tests',


Presumably I'm missing something, but I don't see where this command line argument is being used.

Line 49, above:

wpt/tools/manifest/update.py

Line 49 in b4aa684

paths_to_update=kwargs['tests'],

gsnedders · 2024-12-23T21:45:26Z

tools/manifest/update.py

@@ -45,6 +46,7 @@ def update_from_cli(**kwargs: Any) -> None:
    manifest.load_and_update(tests_root,
                             path,
                             kwargs["url_base"],
+                             paths_to_update=kwargs['tests'],


Nit: The rest of this file seems to use double-quotes. Please match the local style.

gsnedders · 2024-12-23T21:48:51Z

tools/manifest/vcs.py

                 manifest_path: Optional[Text] = None,
                 rebuild: bool = False) -> None:
        self.tests_root = tests_root
        self.url_base = url_base
+        self.paths_to_update = paths_to_update or ['']


Defaulting to [''] seems slightly surprising. Do we not want to default to [self.tests_root]?

This is relevant to the other comments in __iter__. Similar to the response there, I'd like to keep this relative to the tests root.

gsnedders · 2024-12-23T21:50:42Z

tools/manifest/vcs.py

+
+    def __iter__(self) -> Iterator[Tuple[Text, Optional[Text], bool]]:
+        for path_to_update in self.paths_to_update:
+            path = os.path.join(self.tests_root, path_to_update)


Do we want to be joining to tests_root, or do we want to be joining to the current working directory? Certainly they'll frequently be the same, but this feels maybe slightly surprising?

From the CLI, I'd expect to be able to pass an absolute path in here and have it work — or give an error if it wasn't within the test root.

I would like path_to_update to be relative to tests_root.

If we want to accept absolute paths from CLI, I think we can handle that when process the command line arguments. But after that we only need to keep the part relative to tests_root, as paths out of test root can not be handled.

We eventually want to yield something that is relative to tests_root, so keep path_to_update relative to the tests_root seems more natural?

gsnedders · 2024-12-23T22:02:48Z

tools/manifest/vcs.py

-    def __iter__(self) -> Iterator[Tuple[Text, Optional[Text], bool]]:
+    def _make_file_info(self,
+                        path: Text,
+                        path_stat) -> Tuple[Text, Optional[Text], bool]:


Suggested change

path_stat) -> Tuple[Text, Optional[Text], bool]:

path_stat: stat_result) -> Tuple[Text, Optional[Text], bool]:

To run WPTs locally, developers usually only run a few of tests each time. Updating WPT manifest for the whole WPT Repo is not necessary and took ~3 seconds on a performant develop machine. And this can take forever if the developer is using a remote file system. This change allows updating WPT manifest for tests only intended to be run. This brings down the test time for a few tests to ~0.5 seconds. Additional work is needed to plug in this into the test runner.

WeizhongX · 2025-01-02T23:27:59Z

I think this needs some tests

I tried to use pyfakefs, then test manifest.load_and_update. This turns out to be difficult because pyfakefs always returns str (or whatever is used when creating the fake files), but the real code uses both str and bytes.

Do you have any suggestion on this? @jgraham

WeizhongX · 2025-01-06T19:03:45Z

@gsnedders can you take a look at the updates? Thanks!

Coments addressed

* Allow patially update to wpt manifest To run WPTs locally, developers usually only run a few of tests each time. Updating WPT manifest for the whole WPT Repo is not necessary and took ~3 seconds on a performant develop machine. And this can take forever if the developer is using a remote file system. This change allows updating WPT manifest for tests only intended to be run. This brings down the test time for a few tests to ~0.5 seconds. Additional work is needed to plug in this into the test runner.

wpt-pr-bot added infra manifest labels Aug 13, 2024

wpt-pr-bot assigned DanielRyanSmith Aug 13, 2024

wpt-pr-bot requested review from DanielRyanSmith, foolip and jgraham August 13, 2024 17:25

WeizhongX closed this Nov 4, 2024

WeizhongX reopened this Nov 4, 2024

moz-wptsync-bot added the mozilla:gecko-blocked label Nov 4, 2024

WeizhongX force-pushed the partial-update-manifest branch 2 times, most recently from 26d192f to 7f8fa87 Compare November 4, 2024 23:39

WeizhongX requested a review from jonathan-j-lee November 5, 2024 21:48

jonathan-j-lee approved these changes Nov 5, 2024

View reviewed changes

jgraham previously requested changes Nov 7, 2024

View reviewed changes

WeizhongX changed the title ~~Allow patially update to wpt manifest~~ Allow patial update to wpt manifest Nov 7, 2024

WeizhongX force-pushed the partial-update-manifest branch 2 times, most recently from f3bc9aa to b4aa684 Compare November 13, 2024 21:19

gsnedders reviewed Dec 23, 2024

View reviewed changes

WeizhongX force-pushed the partial-update-manifest branch from b4aa684 to bd7e8be Compare December 31, 2024 00:16

update per review comments

f367c07

WeizhongX force-pushed the partial-update-manifest branch from bd7e8be to f367c07 Compare January 2, 2025 18:47

WeizhongX requested a review from jgraham January 6, 2025 19:02

WeizhongX merged commit 947ece2 into master Jan 8, 2025
33 checks passed

WeizhongX deleted the partial-update-manifest branch January 8, 2025 17:44

moz-wptsync-bot removed the mozilla:gecko-blocked label Jan 9, 2025

mbrodesser-Igalia mentioned this pull request Jan 28, 2025

AddTestCheckingLangAttributeAffectsLigatures mbrodesser-Igalia/wpt#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow patial update to wpt manifest #47588

Allow patial update to wpt manifest #47588

WeizhongX commented Aug 13, 2024 •

edited

Loading

WeizhongX commented Nov 5, 2024

jonathan-j-lee left a comment

jonathan-j-lee Nov 5, 2024

jonathan-j-lee Nov 5, 2024

jgraham left a comment

jgraham Nov 7, 2024

WeizhongX Nov 7, 2024

gsnedders Dec 23, 2024

WeizhongX commented Nov 7, 2024

WeizhongX commented Nov 14, 2024

gsnedders Dec 23, 2024

gsnedders Dec 23, 2024

gsnedders Dec 23, 2024

WeizhongX Dec 31, 2024

gsnedders Dec 23, 2024

WeizhongX Dec 27, 2024

WeizhongX Dec 27, 2024

gsnedders Dec 23, 2024

WeizhongX commented Jan 2, 2025

WeizhongX commented Jan 6, 2025

	help='Paths of test files or directories to update.')
	help=('Test files or directories to update. '
	'Omit to update all items under the test root.'))

	path_stat) -> Tuple[Text, Optional[Text], bool]:
	path_stat: stat_result) -> Tuple[Text, Optional[Text], bool]:

Allow patial update to wpt manifest #47588

Allow patial update to wpt manifest #47588

Conversation

WeizhongX commented Aug 13, 2024 • edited Loading

WeizhongX commented Nov 5, 2024

jonathan-j-lee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgraham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeizhongX commented Nov 7, 2024

WeizhongX commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeizhongX commented Jan 2, 2025

WeizhongX commented Jan 6, 2025

WeizhongX commented Aug 13, 2024 •

edited

Loading