Status

Tree Management Graveyard
TBPL
--
major
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: emorley, Assigned: emorley)

Tracking

Dependency tree / graph

Details

Attachments

(4 attachments)

(Assignee)

Description

6 years ago
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>
(Assignee)

Comment 1

6 years ago
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(/\/[^\/]+$/, '/');
(Assignee)

Comment 2

6 years ago
Created attachment 637144 [details] [diff] [review]
A - Rename absolute*URL to prod*URL

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)
(Assignee)

Updated

6 years ago
Attachment #637144 - Attachment description: Part 1 → A - Rename absolute*URL to prod*URL
(Assignee)

Comment 3

6 years ago
Created attachment 637149 [details] [diff] [review]
B - Break out absoluteBaseURL calculation

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)
(Assignee)

Comment 4

6 years ago
Created attachment 637150 [details] [diff] [review]
C - Make reftestLogURL an absolute URL

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)
(Assignee)

Comment 5

6 years ago
Created attachment 637152 [details] [diff] [review]
D - Clarify baseURL

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+
(Assignee)

Updated

6 years ago
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.