Bug 64300

Summary: TestFailures page's new-bug-filing code is a mess!
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Tools / TestsAssignee: Adam Roben (:aroben) <aroben>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dbates, ddkilzer
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
This time with unit tests! abarth: review+, aroben: commit-queue?

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.