Bug 248745

Summary: In results.html, links to diffs for tests with periods in the name fail
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: Tools / TestsAssignee: Jonathan Bedard <jbedard>
Status: RESOLVED FIXED    
Severity: Normal CC: jbedard, pascoe, philn, simon.fraser, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=238832

Description Simon Fraser (smfr) 2022-12-04 10:13:01 PST 
When a test like http/wpt/service-workers/fetch-service-worker-preload-download.https.html fails, the expected, actual, diff and pretty diff links in results. html are broken. For example, they link to:
file://null/Volumes/Data/Development/system/webkit/OpenSource/WebKitBuild/Debug/layout-test-results/http/wpt/service-workers/fetch-service-worker-preload-download.https.html-pretty-diff.html
instead of:
file://null/Volumes/Data/Development/system/webkit/OpenSource/WebKitBuild/Debug/layout-test-results/http/wpt/service-workers/fetch-service-worker-preload-download.https-pretty-diff.html
Comment 1 Simon Fraser (smfr) 2022-12-04 10:16:50 PST 
There was an attempt to fix in bug 238832. but that broke results for SVG which has periods in folder names.
Comment 2 Philippe Normand 2022-12-05 03:39:53 PST 
The issue is that for this testName we end up in the "Temporary fix" case for which I have no context. 

    static testPrefix(testName)
    {
        let parts = Utils.splitExtension(testName);
        let prefix = parts[0];
        let remains = parts[2];
        if (remains) {
            if (remains.includes('?'))
                prefix += '_' + remains.split('?')[1]
            else if (remains.includes('#'))
                prefix += '_' + remains.split('#')[1]
        } else if (Utils.splitExtension(parts[0])[1].length > 5) {
            // Temporary fix, also in Tools/Scripts/webkitpy/layout_tests/constrollers/test_result_writer.py, line 115.
            // FIXME: Refactor to avoid confusing reference to both test and process names.
            return testName;
        }
        return prefix;
    }

Utils.splitExtension(parts[0])[1] == '.https' here, so we'd need an additional check, but as I lack the historical meaning of this code, I'm not sure how to fix this.
Comment 3 Simon Fraser (smfr) 2022-12-05 11:36:53 PST 
It should be easy to handle both a period in the test file name, and a period in a directory name, which I think is all that's required to handle this?
Comment 4 Jonathan Bedard 2022-12-06 14:03:13 PST 
Figured out the problem, results.html is using `Utils.splitExtension(parts[0])[1].length > 5` instead of `Utils.splitExtension(parts[0])[1].length - 1 > 5`, which is what the matching Python code does. I don't have a great answer for the "why", though. Probably worth the naive fix for know and sorting out the existing FixMe in the near future.
Comment 5 Radar WebKit Bug Importer 2022-12-06 14:03:26 PST 
<rdar://problem/103041235>
Comment 6 Jonathan Bedard 2022-12-06 14:06:08 PST 
Pull request: https://github.com/WebKit/WebKit/pull/7222
Comment 7 EWS 2022-12-07 17:27:57 PST 
Committed 257527@main (fc8aae223895): <https://commits.webkit.org/257527@main>

Reviewed commits have been landed. Closing PR #7222 and removing active labels.