Closed
Bug 774942
Opened 12 years ago
Closed 10 years ago
Data.js should separate out regular expressions into a JSON file
Categories
(Tree Management Graveyard :: TBPL, defect)
Tree Management Graveyard
TBPL
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: k0scist, Unassigned)
Details
Attachments
(1 file, 2 obsolete files)
11.31 KB,
patch
|
emorley
:
feedback-
|
Details | Diff | Splinter Review |
From http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/raw-file/tip/js/Data.js: // see Config.testNames and Config.buildNames var type = /talos.*xperf$/i.test(name) ? "Talos xperf" : /talos.*tsspider$/i.test(name) ? "Talos sspider" : /talos.*twinopen$/i.test(name) ? "Talos winopen" : /talos.*tpan$/i.test(name) ? "Talos pan" : /talos.*tdhtml$/i.test(name) ? "Talos dhtml" : This walks through a bunch of regexes and tries to match each of them in turn. It would be more consumable if instead it used a JSON structure in a separate file: {"Talos xperf": "talos.*xperf", "Talos sspider": "talos.*tsspider", ... etc }
Comment 1•12 years ago
|
||
This change looks sane enough to me, but I'm not sure if I'm fetching the file from the right place or even how to test the whole thing. Advice would be appreciated. I chose sync XHR so as to not massively rewrite all of this.
Attachment #660068 -
Flags: feedback?(jhammel)
Comment 2•12 years ago
|
||
Attachment #660068 -
Attachment is obsolete: true
Attachment #660068 -
Flags: feedback?(jhammel)
Attachment #660090 -
Flags: feedback?(jhammel)
Updated•12 years ago
|
Attachment #660090 -
Flags: feedback+
Reporter | ||
Comment 3•12 years ago
|
||
Comment on attachment 660090 [details] [diff] [review] patch This looks good to me and allows me to get the information I need to get, but I'm not a TBPL peer.
Attachment #660090 -
Flags: feedback?(jhammel) → feedback+
Comment 4•12 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #1) > or even how to test the whole thing Testing client-side changes is just a case of changing a Config.js pref and loading from the local filesystem :-) http://hg.mozilla.org/users/mstange_themasta.com/tinderboxpushlog/file/tip/README#l21
Updated•12 years ago
|
Attachment #660090 -
Flags: feedback?(mstange)
Comment 5•12 years ago
|
||
Comment on attachment 660090 [details] [diff] [review] patch Looks good, except for the async: false part. If there's an simple way to load the file asynchronously while making sure that it's loaded before any calls to getMachine are made, I'd prefer that.
Attachment #660090 -
Flags: feedback?(mstange) → feedback+
Comment 6•12 years ago
|
||
Here's a patch that moves the load up into Controller.init and delays the code there until after the load completes. It's not obvious to me that this is any better, since this delays setting up the whole UI--and doesn't display the UI if the file can't be found!--but I am not familiar with the code to find a happy medium. I didn't use loadTracker because I wasn't sure how calling bits in loadTracker would interact with Controller's initialization not being done yet.
Attachment #660090 -
Attachment is obsolete: true
Attachment #660455 -
Flags: feedback?(mstange)
Comment 7•12 years ago
|
||
Thinking about this - isn't it just going to break the ability for TBPL to run locally? (using the method in comment 4) We could of course use |url: baseURL + "TestRegexpsMap.json"|, however that will prevent me from easily testing regex changes in the future. As such, we need to get this working from the local filesystem before it can land, if that's ok?
Comment 8•12 years ago
|
||
(In reply to Ed Morley (Away 18th-20th) [:edmorley UTC+1] from comment #7) > As such, we need to get this working from the local filesystem before it can > land, if that's ok? Sure! No sense in fixing bugs if you just break other useful things.
Comment 9•12 years ago
|
||
I don't think there's a good way to arrange things so the local filesystem and the server cases both work. How about this hack instead: We format the JSON specially so it can be extracted by interested parties, something like: // Explanatory comment about the peculiar formatting. var TestRegexpsMap = // JSON BEGIN MARKER { ...whatever information needed... ...note that we can't have comments in here... } // JSON END MARKER ; Then any interested parties can retrieve the .js file where this is stored (from hg or from tbpl itself) and locate the relevant JSON straightforwardly. It's pragmatic, it's not pretty. Opinions?
Reporter | ||
Comment 10•12 years ago
|
||
FWIW, the current format is readable in some horrible manner: http://k0s.org/mozilla/hg/talosnames/file/277b167060e5/talosnames/api.py#l82 Its horrible, unreliable, and horrible. I'd rather move towards something canonical and sensible. I'd rather not do this in a hacky way.
Comment 11•12 years ago
|
||
Actually, why don't we make the list part of Config.js? Then we get synchronous loading for free because <script>s always wait for each other.
Comment 12•12 years ago
|
||
(In reply to Markus Stange from comment #11) > Actually, why don't we make the list part of Config.js? Then we get > synchronous loading for free because <script>s always wait for each other. I don't see how that addresses the issue of trying to make the regexes available via JSON.
Comment 13•12 years ago
|
||
It wouldn't; is that a goal? Comment 0 talks about making it consumable, so maybe it is... what would be the consumers? I guess I'm not clear on the purpose of this bug. If we just want to make things more readable and easier to find when changing the code, we don't need to make the list externally available.
Reporter | ||
Comment 14•12 years ago
|
||
The goal is to have something consumable to other pieces of software so that interoperability with TBPL may be ensured. Let's say you're writing a piece of software that wants to see the TBPL name (and letter, which is keyed off the name separately) from the buildbot suite name (the thing the regex matches against). See comment 10 for how I do this now.
Comment 15•12 years ago
|
||
Comment on attachment 660455 [details] [diff] [review] patch Oh, I missed that. OK, so the Config.js / <script> approach won't fly. I think the current patch is good, except for the objection about running from the local file system. I don't have an opinion on that so I'm passing the baton to edmorley.
Attachment #660455 -
Flags: feedback?(mstange) → feedback?(bmo)
Comment 16•12 years ago
|
||
Comment on attachment 660455 [details] [diff] [review] patch We really can't break testing from the local filesystem; it will be an immense pain for me to have to fire up a VM every time I want to work on TBPL's frontend - and it increases the level of effort required from new contributors wanting to help out (I'm looking to get TBPL added to http://whatcanidoformozilla.org soon, so this will be an even bigger issue then).
Attachment #660455 -
Flags: feedback?(bmo) → feedback-
Comment 17•12 years ago
|
||
(In reply to Ed Morley [:edmorley UTC+0] from comment #16) > and it increases the level of effort required from new > contributors wanting to help out eg the new contributor in bug 683833.
Assignee | ||
Updated•10 years ago
|
Product: Webtools → Tree Management
Comment 18•10 years ago
|
||
Wontfix since TBPL is being replaced by Treeherder.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
Assignee | ||
Updated•9 years ago
|
Product: Tree Management → Tree Management Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•