498679 - Run Addons tests automatically on upload

Status: VERIFIED FIXED

(addons.mozilla.org Graveyard :: Admin/Editor Tools, defect)

Description

[RJ Walsh [:rjwalsh]]

Rework the current uploader to enable running tests on upload.  This will involve creating an ajax framework to run multiple tests asynchronously, following the test hierarchy given in bug 498673.

Comment 1

[RJ Walsh [:rjwalsh]]

Attachment: Initial Patch for auto-uploads

This is just a first shot - definitely not final.  Any ideas for improvements, particularly on the UI end of things are welcome.

Comment 2

[Wil Clouser [:clouserw]]

Comment on attachment 391956 [details] [diff] [review]
Initial Patch for auto-uploads

Functionally it works.  It's just like you said, the UI.

If any UX experts want to chime in, feel free.  Otherwise, off the top of my head:

We can either show the tests as an afterthought or actually block on them, and I think blocking is probably the right thing to do.  Something like:

- Author uploads add-on
- We show a screen running tests
- We say all tests pass.  This can be the same screen as "Add-on Created!" right now
- If all tests didn't pass we show the validation results with a message at the top explaining what they are and the option to either upload a new copy of it with the fixes or continue anyway.

What do you all think of that?

Comment 3

[Justin Scott [:fligtar]]

Sounds lovely. "Your add-on has been uploaded and is now being tested for common problems." -> test results and success page.

Comment 4

[RJ Walsh [:rjwalsh]]

Attachment: Take 2

New workflow: validation results now come before the final completion page.  There was also a lot of work done to allow re-uploading of files when the user chooses to do so.

One major thing missing from this patch is failure handling and how to deal with it.  Right now, the add-on/version/file is still accepted if there are failures outside of the critical test.  The page tells them to upload a newer version, but doesn't keep the add-on off AMO.

The exception is tests in the first group.  Any failure here (which includes current blacklists based on the old methods) will not result in the add-on being created.

Comment 5

[Wil Clouser [:clouserw]]

Comment on attachment 394564 [details] [diff] [review]
Take 2

This had more failed hunks than successful ones when I tried to patch it.  Maybe it suffered from the L10n migration?  Sorry for causing bitrot. :-/

Comment 6

[RJ Walsh [:rjwalsh]]

Attachment: Take 2, SVN up'd

Comment 7

[Wil Clouser [:clouserw]]

Comment on attachment 395085 [details] [diff] [review]
Take 2, SVN up'd

Code looks good so far, but the L10n needs updating to use the new style.

Comment 8

[RJ Walsh [:rjwalsh]]

Attachment: Take 2, with new L10n

Comment 9

[Wil Clouser [:clouserw]]

Comment on attachment 395356 [details] [diff] [review]
Take 2, with new L10n

We talked on IRC, but r- due to:

uploading a new version to an existing add-on creates the version/tests entry.  This means if a user comes back later and attempts to upload the version is says it already exists.  We shouldn't be storing anything until tests are passing or user says "do it anyway."

Comment 10

[RJ Walsh [:rjwalsh]]

Attachment: Take 3, now not storing anything until everything validates

So a lot of the previous patch was thrown out in this version, which means I'm basically back to soliciting feedback.  This is by far better than the other two approaches, but it can still use a good review.  There was a lot of code moved around to get this working, so in particular any regressions spotted would be immensely helpful.

Comment 11

[RJ Walsh [:rjwalsh]]

Attachment: Take 3, SVN up'd

Comment 12

[RJ Walsh [:rjwalsh]]

Attachment: Take 3, fixed with problem from svn up

Yay bitrot!

Comment 13

[RJ Walsh [:rjwalsh]]

Attachment: Wrong file on previous patch

So used to choosing that file while testing :)

Comment 14

[Wil Clouser [:clouserw]]

Comment on attachment 395617 [details] [diff] [review]
Wrong file on previous patch

There is a left over pr() in the code

Trying to upload a new file to an existing version when no other files exist gets me:
document.getElementById("upload-frame").contentWindow.document.getElementById("json") is null /js/developers.js
Line: 144

After uploading an add-on and clicking "continue" on the results the page says "...marked as ." (it's missing a string) and the "add release notes" button on that page is a 404.

Comment 15

[RJ Walsh [:rjwalsh]]

Attachment: v4

I can't reproduce the second issue from the previous comment, but there were some other problems I spotted so hopefully this fixes them.

Comment 16

[Wil Clouser [:clouserw]]

Comment on attachment 395680 [details] [diff] [review]
v4

There is a lot of code like:
+                if ($existing = $this->Addon->findAll("Addon.guid='{$addon['Addon']['guid']}'")) {

that doesn't look restricted to certain models.  I'd feel more comfortable if that was explicit on what cake was binding.

 $data = $this->Addon->query("SELECT `value` FROM `test_results_cache` WHERE `key` = '{$filename}' AND `test_case_id` = -1");
$filename is completely unescaped from $_GET here as far as I can tell.  There is at least one other place where you use $filename in a query but I didn't dig through where it is.  You've got to escape all user input in queries.

Comment 17

[RJ Walsh [:rjwalsh]]

Attachment: v5

Fixes from previous comment, with a few more I caught myself.

Comment 18

[Wil Clouser [:clouserw]]

Comment on attachment 395722 [details] [diff] [review]
v5

I'm r+ing, but 2 minor things to fix before committing:

1) When adding a file to an existing version, clicking "continue" after the tests, the first sentence is:  Your new file has been added to version 0.13 and is currently marked as . (again, missing a word).

2) On the testing page, when there is a warning, "validation help page" should be a link.

--------------

All that said, this is a really big change to core functionality of the site.  QA: let's hit this hard.

Comment 19

[RJ Walsh [:rjwalsh]]

Alright, committed in revs r49665 (code) and r49666 (migration).  

I second the QA aspect - things seem OK on my end, but there is definitely potential for stuff I unknowingly changed.  Feel free to assign any bugs that arise to me.

I'm marking this as resolved - anything that comes up related here can just be a separate bug, unless someone sees the need to re-open this.

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED when I tested bug 512135 and bug 441852.