Closed Bug 768474 Opened 12 years ago Closed 12 years ago

Reftest analyser broken

Categories

(Tree Management Graveyard :: TBPL, defect)

Product:
Tree Management Graveyard
Component:
TBPL
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: emorley, Assigned: emorley)

References

Details

Attachments

(4 files)

A - Rename absolute*URL to prod*URL
3.31 KB, patch
mstange
: review+
Details | Diff | Splinter Review
B - Break out absoluteBaseURL calculation
2.74 KB, patch
mstange
: review+
Details | Diff | Splinter Review
C - Make reftestLogURL an absolute URL
1.39 KB, patch
mstange
: review+
Details | Diff | Splinter Review
D - Clarify baseURL
1.20 KB, patch
mstange
: review+
Details | Diff | Splinter Review 
Constant "Fetching reftest log failed." when trying to use the reftest analyser.

Can only think this is due to bug 762120; but yet it worked when I tested using Vagrant booo :-(

eg:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=de70e79ced32

Reftest analyser link unescaped:

data:text/html,<!DOCTYPE html><title>Loading reftest analyzer for Mozilla-Inbound Rev3 Fedora 12x64 mozilla-inbound opt test reftest</title><link rel="stylesheet" href="https://tbpl.mozilla.org/css/style.css"><body><p class="loading">Retrieving reftest log...</p><script src="https://tbpl.mozilla.org/js/jquery.js"></script><script src="https://tbpl.mozilla.org/js/NetUtils.js"></script><script>
function encode(s) { return escape(s).replace(/=/g, "%3d") }
NetUtils.loadText("php\/getLogExcerpt.php?id=12998274&type=reftest",
                   function(log) { window.location.replace("https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#log=" + encode(encode(log))) },
                   function() { $("p").removeClass("loading").text("Fetching reftest log failed.") },
                   function() { $("p").removeClass("loading").text("Fetching reftest log timed out.") });
</script>
Ah, I know why this worked when testing in Vagrant; I was testing side-by-side with the local filesystem version and hadn't popped the mq patch that I use to set baseURL.

Ok, so:

The patch in bug 762120 did:
-          'NetUtils.loadText(' + self._makeJSString(baseURL + result.reftestLogURL) + ',\n' +
+          'NetUtils.loadText(' + self._makeJSString(result.reftestLogURL) + ',\n' +

I had read baseURL as Config.baseURL, which would have meant it was an empty string on prod/vagrant so they wouldn't have been affected by this patch. However unhelpfully, baseURL is actually used for the absolute base URL in UserInterface.js (which is different from Config.js's usage of it):
var baseURL = Config.baseURL || document.baseURI.replace(/\/[^\/]+$/, '/');
In one of the later patches, I'd like to use 'absoluteBaseURL' for the absolute equivalent of baseURL. The current usage is more about ensuring URLs posted in bug comments are publicly accessible/point to a production instance, so this renames them as such. 

Happy to adjust 'prod' to 'public' or anything else you can think of that would be better.
Assignee: nobody → bmo
Status: NEW → ASSIGNED
Attachment #637144 - Flags: review?(mstange)
Attachment #637144 - Attachment description: Part 1 → A - Rename absolute*URL to prod*URL
We're going to need to calculate the absolute URL in both UserInterface.js & BuildbotDBUser.js, so it makes sense to break it out into Config.js with the other URLs. Since it is a calculated value, I've added it to the bottom of Config.js alongside the build/test name generated variables.
Attachment #637149 - Flags: review?(mstange)
Makes reftestLogURL use the new calculated absoluteBaseURL, so as to make it work both from the local filesystem & when the backend is hosted locally. Fixes the regression from bug 762120.
Attachment #637150 - Flags: review?(mstange)
Adds a small note to baseURL, since now we have absoluteBaseURL and prodBaseURL a bit of extra clarification is probably needed. Happy to adjust wording if you can think of something better :-)

--

Have tested these four patches using Vagrant (this time popping the baseURL mq, so testing properly) and also from the local filesystem using baseURL.
Attachment #637152 - Flags: review?(mstange)
Attachment #637144 - Flags: review?(mstange) → review+
Attachment #637149 - Flags: review?(mstange) → review+
Attachment #637150 - Flags: review?(mstange) → review+
Attachment #637152 - Flags: review?(mstange) → review+
Depends on: 769169
Product: Webtools → Tree Management
Product: Tree Management → Tree Management Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: