search

web-platform-tests / wpt.fyi

web-platform-tests dashboard
https://wpt.fyi/
Other
190 stars 90 forks source link

diff view doesn't show removed tests/directories #1886

Open stephenmcgruer opened 4 years ago

stephenmcgruer commented 4 years ago

https://github.com/web-platform-tests/wpt/pull/22875 is a PR that moved some tests from one directory to another. The generated diff link from GitHub was https://wpt.fyi/results/?diff&filter=ADC&run_id=478940001&run_id=478930001, which shows:

image

(For a second as it loads, you can see video-raf/ flicker in and out of existence)

The non-diff view does show both directories: https://wpt.fyi/results/?run_id=478940001&run_id=478930001

image

stephenmcgruer commented 4 years ago

Actually, it gets weird. I just noticed that video-rvfc/ actually reports 23/29 in the old run, and reports the same in the file overview:

image

But click once more, into a test, and you get:

image

Hexcles commented 4 years ago

This is indeed rather confusing. We have had 3 ways of calculating the diff in total:

  1. (No longer used, and hopefully completely removed) diffing happens purely in the front end (JavaScript comparing two runs and calculating deltas)
  2. (No longer default) the searchcache (/api/search) takes an additional flag to return the diff in addition to results (this is faster and the implementation is cleaner than 1).
  3. (Current default) a new API /api/diff that takes the results from the searchcache and applies some post-processing; most notably it queries GitHub for renames.

What you are seeing demonstrates two bugs: a. There's a flash of content from 2 to 3 in the front end even though 3 is the default. (You can see both API calls in Devtools). The feature control is likely broken somewhere. b. Even though 3 correctly detects the renaming of the directory, the UI is confusing: 1) it should really show something like "video-raf -> video-rvfc" in the directory view 2) the file view fails to show the results from the original (pre-renaming) test.

stephenmcgruer commented 4 years ago

(No longer used, and hopefully completely removed) diffing happens purely in the front end (JavaScript comparing two runs and calculating deltas) (No longer default) the searchcache (/api/search) takes an additional flag to return the diff in addition to results (this is faster and the implementation is cleaner than 1).

I will advocate again for removing all this code we have behind flags, whether because it has been replaced by something newer or because it just never launched - all that code does is bit-rot and cause bugs. If we have a plan to launch something we should follow through, else we should remove it.

See https://chromium.googlesource.com/chromium/src/+/refs/heads/master/docs/flag_ownership.md, especially "I Don't Want My Flag To Expire!" and "What Should My Expiry Be?" :)