Bug 64300 – TestFailures page's new-bug-filing code is a mess!

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Adam Roben (:aroben)]

TestFailures page's new-bug-filing code is a mess!

[Adam Roben (:aroben)]

Created attachment 100330 [details]
Patch

[Adam Roben (:aroben)]

Created attachment 100373 [details]
This time with unit tests!

[Adam Barth]

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.

[Adam Barth]

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.)

[Adam Roben (:aroben)]

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.

[Adam Roben (:aroben)]

(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.

[Adam Roben (:aroben)]

Committed r90814: <http://trac.webkit.org/changeset/90814>

[Adam Barth]

> >> 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.