Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use MANIFEST.json as the ground truth for validating and displaying results #56

Open
foolip opened this issue Apr 17, 2018 · 10 comments
Open
Labels
enhancement New feature or request

Comments

@foolip
Copy link
Member

foolip commented Apr 17, 2018

Moved from web-platform-tests/results-collection#123.

Two related problems:

  1. Some test suites have a lot of manual tests, and making this clear would be useful. Example: https://wpt.fyi/fullscreen/ vs. https://github.com/w3c/web-platform-tests/tree/master/fullscreen
  2. Not all runners will run all test types (especially wdspec) and may have bugs that cause certain tests to have no recorded results at all.

These could be solved by somehow using MANIFEST.json for each commit, or information derived from it. That could also be used to do something more useful for https://wpt.fyi/does/not/exist/, although that's probably already doable, just not implemented.

@foolip
Copy link
Member Author

foolip commented Apr 17, 2018

The discussion in web-platform-tests/results-collection#98 might also be relevant to this, although we could at most align the tests, not the subtests.

@lukebjerring
Copy link
Contributor

From web-platform-tests/results-collection#123, @gsnedders said:

That relies on everyone implementing runners correctly. I think there's definitely an argument that it's the collection side that should handle it?

There's an imbalance here with WPT run not working cross-platform that means we really are likely to have multiple runner implementations (already do for Edge, Content Shell). I feel like if we can't necessarily trust the clients/runners to do the right thing, then the step for fleshing out "missing"/manual/un-run tests as per manifest can be shoved server-side, but I still think it needs to be (implicit) in the results JSON blobs, not implemented by storing a copy of the as-was manifest for every run (or retrospectively loading it from the repo, or other bug-prone approaches).

i.e. It should be part of the post-processing of the results receiver.

@foolip
Copy link
Member Author

foolip commented Apr 23, 2018

@Hexcles, WDYT, would it make sense for the results receiver to combine information from the run and the manifest to produce the final form suitable for wpt.fyi?

@Hexcles
Copy link
Member

Hexcles commented Apr 23, 2018

I agree with @gsnedders and @lukebjerring here.

The benefits of implementing this inside the results-receiver is two-fold:

  • Reducing the total complexity (implement once as opposed to in all the runners; and we know there are already >1 runners.)
  • Not requiring runners to be 100% reliable (results-receiver would be the final catch-all check; also, fewer implementations lead to lower possibility of bugs and faster bug fixes.)

I'd like to implement this together with "validating the incoming results (against manifest)" in the second iteration (the first batch of non-crucial features).

@foolip
Copy link
Member Author

foolip commented Apr 24, 2018

@Hexcles can you rename this issue to better describe the solution you have in mind?

@Hexcles Hexcles changed the title Use MANIFEST.json to show manual tests and tests with no results Use MANIFEST.json as the ground truth for validating and displaying results Apr 25, 2018
@Hexcles
Copy link
Member

Hexcles commented Apr 25, 2018

The problem is twofold:

  1. Use manifest to validate results and calculate completeness.
  2. Listing tests based on manifest. If a test doesn't have result (from any browser), including manual tests, put a placeholder N/A in the UI.

The two goals can be achieved in the same process. In the results processor, we iterate over the tests listed in the manifest and try to find the corresponding results from the wptreport JSON (instead of the other way around).

And we must make sure we have the correct manifest (at the right revision). I imagine (and certainly wish) all test runs happen at PR merge points. If so, we can easily download the exact manifest from GitHub releases.

@Hexcles Hexcles self-assigned this Apr 25, 2018
@foolip
Copy link
Member Author

foolip commented Apr 25, 2018

Could the results receiver use wpt manifest locally? That will download when possible and otherwise generate.

@Hexcles
Copy link
Member

Hexcles commented Apr 25, 2018 via email

@foolip
Copy link
Member Author

foolip commented Apr 25, 2018

Sounds like it was named appropriately :)

@foolip
Copy link
Member Author

foolip commented Sep 25, 2019

I've been working in https://github.com/foolip/wpt-stats to see if we can ensure there are manifests available for download for at least all master runs, and I've filed web-platform-tests/wpt#19311 for a few exceptions where that's not the case.

@Hexcles I'm not sure this is really how you want to get the manifest. Generating one in the receiver is probably doable, but I think there will be some commits which aren't reachable from any ref. Should we instead require a manifest to be submitted together with the reports? Or fall back to a manifest that's close in the commit graph?

@Hexcles Hexcles removed their assignment Dec 17, 2020
stephenmcgruer added a commit that referenced this issue Mar 10, 2021
The use of the manifest in the frontend never shipped, and the flags are
off in prod and staging. This code is non-trivial and removing it makes
the code easier to understand.

The /api/manifest endpoint is not removed because it is still used by
Bikeshed[0].

[0]: https://github.com/tabatkins/bikeshed/blob/e797b9548c0bd140c9b1500809869b09f50f7d99/bikeshed/update/updateWpt.py

Updates #56
stephenmcgruer added a commit that referenced this issue Mar 10, 2021
The use of the manifest in the frontend never shipped, and the flags are
off in prod and staging. This code is non-trivial and removing it makes
the code easier to understand.

The /api/manifest endpoint is not removed because it is still used by
Bikeshed[0].

[0]: https://github.com/tabatkins/bikeshed/blob/e797b9548c0bd140c9b1500809869b09f50f7d99/bikeshed/update/updateWpt.py

Updates #56
stephenmcgruer added a commit that referenced this issue Mar 15, 2021
The use of the manifest in the frontend never shipped, and the flags are
off in prod and staging. This code is non-trivial and removing it makes
the code easier to understand.

The /api/manifest endpoint is not removed because it is still used by
Bikeshed[0] and the GitHub checks in wpt-metadata[1]

[0]: https://github.com/tabatkins/bikeshed/blob/e797b9548c0bd140c9b1500809869b09f50f7d99/bikeshed/update/updateWpt.py
[1]: https://github.com/web-platform-tests/wpt-metadata/blob/557b49eb329670395d90744b084869e848b268ac/test_paths_test.go#L30

Updates #56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants