Bug 64300 - TestFailures page's new-bug-filing code is a mess!
Summary: TestFailures page's new-bug-filing code is a mess!
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Roben (:aroben)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 11:05 PDT by Adam Roben (:aroben)
Modified: 2011-07-12 10:09 PDT (History)
3 users (show)

See Also:


Attachments
Patch (20.82 KB, patch)
2011-07-11 11:06 PDT, Adam Roben (:aroben)
no flags Details | Formatted Diff | Diff
This time with unit tests! (42.50 KB, patch)
2011-07-11 15:49 PDT, Adam Roben (:aroben)
abarth: review+
aroben: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2011-07-11 11:05:31 PDT
TestFailures page's new-bug-filing code is a mess!
Comment 1 Adam Roben (:aroben) 2011-07-11 11:06:25 PDT
Created attachment 100330 [details]
Patch
Comment 2 Adam Roben (:aroben) 2011-07-11 15:49:53 PDT
Created attachment 100373 [details]
This time with unit tests!
Comment 3 Adam Barth 2011-07-11 16:10:57 PDT
Comment on attachment 100373 [details]
This time with unit tests!

View in context: https://bugs.webkit.org/attachment.cgi?id=100373&action=review

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBugForm.js:35
> +        var formData = {
> +            product: 'WebKit',
> +            version: '528+ (Nightly build)',
> +        };

This seems look odd data to hard-code here.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBugForm.js:62
> +        var form = document.createElement('form');
> +        form.method = 'POST';
> +        form.action = this._bugzilla.baseURL + 'enter_bug.cgi';
> +
> +        for (var key in formData) {
> +            var input = document.createElement('input');
> +            input.type = 'hidden';
> +            input.name = key;
> +            input.value = formData[key];
> +            form.appendChild(input);
> +        }

This whole function is something you can do in like one line of jQuery, for whatever that's worth.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBugForm.js:73
> +    set component(x) {
> +        this._component = x;
> +    },
> +
> +    get component() {
> +        return this._component;
> +    },

Woah, is this really needed?  Why not just expose a real property?

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/TestFailureBugForm.js:35
> +    this.component = 'Tools / Tests';

Ojan would tell you to store all these string as constants and reference them with a symbol.  That has two benefits:

1) If you typo the string, you get a louder error.
2) If we change the components in bugzilla, it's easier to fix all the code.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/TestFailureBugForm.js:48
> +        var form = NewBugForm.prototype.domElement.call(this);

Oh man, you're doing JavaScript inheritance manually.  Ok.  That gets really ugly really fast.

> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/TestFailureBugForm_unittests.js:44
> +    var mockTrac = {};
> +    mockTrac.changesetURL = function(revisionNumber) {
> +        return '[CHANGESET URL r' + revisionNumber + ']';
> +    }
> +    mockTrac.logURL = function(path, startRevision, endRevision) {
> +        return '[LOG URL ' + path + ', r' + startRevision + ', r' + endRevision + ']';
> +    }

You'll eventually want to put these in a place where more folks can use them, but you can do that in the future when needed.
Comment 4 Adam Barth 2011-07-11 16:12:14 PDT
My main impression from reading this code is that it has a lot of detailed dependencies on our current configuration of bugzilla.  If/when those changes, this code is likely to break.  I'm not sure what to do about that, exactly, but factoring those dependencies into some sort of config.js might help somewhat.  (Another option is not to care about that yet, of course.)
Comment 5 Adam Roben (:aroben) 2011-07-12 05:24:48 PDT
Comment on attachment 100373 [details]
This time with unit tests!

View in context: https://bugs.webkit.org/attachment.cgi?id=100373&action=review

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBugForm.js:35
>> +        };
> 
> This seems look odd data to hard-code here.

You're right. I'll move this down to TestFailureBugForm.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBugForm.js:62
>> +        }
> 
> This whole function is something you can do in like one line of jQuery, for whatever that's worth.

Good to know.

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBugForm.js:73
>> +    },
> 
> Woah, is this really needed?  Why not just expose a real property?

It's not really needed. I just wasn't sure how else to represent this class's API. Any suggestions?

>> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/TestFailureBugForm.js:35
>> +    this.component = 'Tools / Tests';
> 
> Ojan would tell you to store all these string as constants and reference them with a symbol.  That has two benefits:
> 
> 1) If you typo the string, you get a louder error.
> 2) If we change the components in bugzilla, it's easier to fix all the code.

OK, I'll add constants.
Comment 6 Adam Roben (:aroben) 2011-07-12 05:25:33 PDT
(In reply to comment #4)
> My main impression from reading this code is that it has a lot of detailed dependencies on our current configuration of bugzilla.  If/when those changes, this code is likely to break.  I'm not sure what to do about that, exactly, but factoring those dependencies into some sort of config.js might help somewhat.  (Another option is not to care about that yet, of course.)

Hopefully this is something that can be cleaned up as we go along.
Comment 7 Adam Roben (:aroben) 2011-07-12 06:53:48 PDT
Committed r90814: <http://trac.webkit.org/changeset/90814>
Comment 8 Adam Barth 2011-07-12 10:09:39 PDT
> >> Tools/BuildSlaveSupport/build.webkit.org-config/public_html/TestFailures/NewBugForm.js:73
> >> +    },
> > 
> > Woah, is this really needed?  Why not just expose a real property?
> 
> It's not really needed. I just wasn't sure how else to represent this class's API. Any suggestions?

One option is to set the properties to null in the constructor.