Merge lp:~henninge/launchpad/bug-824435-failure-reporting-3 into lp:launchpad

[Henning Eggers]

[r=jtv,wallyworld][bug=824435][incr] Add tests for the request builds code in requestbuilds_overlay.js.

= Summary =

The next part in my ongoing quest to replace RequestResponseHandler in
requestbuild_overlay.js with ErrorHandler from client.js. This work was
triggered by the fact that RequestResponseHandler is not only used by
"request daily build" but also by "request builds" which uses a
formoverlay to let the user request builds for this recipe. If I am
going to replace RequestResponseHandlers I need test to make sure that
"request builds" still works as it should after the replacement, just
like I did for "request daily build".

The added complexity here is that "request builds" uses both formoverlay
and the webservice client Launchpad, neither of which had been
instrumented yet to work with mockio.

== Proposed fix ==

Add an io_provider parameter to the Launchpad client class and
instrument its methods.

Add an io_provider attribute to the FormOverlay class. This works out
quite well as attributes can have a default value defined, which in
this case will be "Y". So no extra code is needed to select the right
io_provider.

Wire up io_provider in requestbuild_overlay.js so that it gets passed to
Launchpad and FormOverlay.

Add the test for "request builds".

== Implementation details ==

I made an effort to remove all the js lint that was reported in
requestbuild_overlay.js but that bloated the diff quite a bit. The diff
of the functional changes is found here:
http://pastebin.ubuntu.com/674533/
This diff simply exludes the last four revisions which are purely
concerned with lint. Here is the diff of those:
http://pastebin.ubuntu.com/674550/

I needed to expose a method in requestbuild_overlay.js to destroy the
overlay to be able to isolate the tests in the test suite. Also, the
overlay was clobbered by the failure handler for 5xx errors. I rewrote
that so it displays 5xx errors in the same space of the overlay as other
error messages.

I moved the sample HTML markup to the HTML file and put them inside a
<script type="text/x-template"> tag. This way the browser will ignore
the content inside them and not display them. Also, the nodes inside
the script tags will not show up in YUI node queries. Very handy, I got
that suggestion from the YUI page.

I had to move the io_provider helpers from mockio.js to to a different
location because files inside the "testing" directory are not included
in launchpad.js. I put them in client.js and gave them better names. I
also moved their tests to test_lp_client.js.

function foo() {} creates a global variable foo.
var foo = function() {} does not.
Just in case you might ask. ;-)

== Tests ==

firefox lib/lp/app/javascript/formoverlay/tests/test_formoverlay.html
firefox lib/lp/app/javascript/testing/tests/test_mockio.html
firefox lib/lp/app/javascript/tests/test_lp_client.html
firefox lib/lp/code/javascript/tests/test_requestbuild_overlay.html

== Demo and Q/A ==

=== Verify that requesting build still works. ===

Go to a new recipe and click on "Request build(s)" to request builds.
In the dialog chose just one distroseries and submit the form.
It will close and the new build will appear in the table.
Open the dialog again, select the same distroseries, submit.
You should see an informational message in the lower part of the
dialog.
Close and open the dialog again.
Select the same distroseries again and another one. Submit.
The dialog will not close but the new build is added to the table
on the page.
An informational message will appear.

If you know of ways to trigger 5xx errors, try that, too.
The error will be displayed in the same space as the informational
messages were.

=== Verify Launchpad client and FormOverlay still work ===

As these are used by many pages, some random testing on the page is
advised. Go through some pages and play around with forms that are
displayed as overlays to the page. They all should work like before.
with some

= Launchpad lint =

Hm, these are more files than I changed ...

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/formoverlay/formoverlay.js
  lib/lp/app/javascript/tests/test_lp_client.js
  lib/lp/code/javascript/tests/test_requestbuild_overlay.html
  lib/lp/app/javascript/client.js
  lib/lp/app/javascript/tests/test_lp_client.html
  lib/lp/app/javascript/formoverlay/tests/test_formoverlay.js
  lib/lp/app/javascript/testing/tests/test_mockio.js
  lib/lp/code/javascript/requestbuild_overlay.js
  lib/lp/app/javascript/testing/tests/test_mockio.html
  lib/lp/code/javascript/tests/test_requestbuild_overlay.js
  lib/lp/app/javascript/picker/tests/test_picker_patcher.js
  lib/lp/app/javascript/testing/mockio.js

[j.c.sackett (jcsackett)]

For some reason, after almost an hour, the diff for this mp still has not generated. Given that on IRC it was claimed it was quite large, I wonder if perhaps it is *too* large and that's bogging down the diff generation?

[Henning Eggers (henninge)]

No, the diff is around 1200 lines, I have seen larger. I don't know why the diff is not being generated although I thought I heard that diff generation was broken in some way.
The diff that is linked in the cover letter is what this proposal is about.

[Ian Booth (wallyworld)]

Hi Henning,

This is very cool. I really like the addition of the mock io stuff to the base lp client infrastructure; that's something I have been wanting to see and I'm really grateful you've done it. Thanks also for all the lint clean up.

I really like the tip about adding invisible markup to the html file instead of constructing it in the js test code. There's a number of great new features in here - have you considered updating the yui test page on the wiki?

I have a question - if a build request returns with a response status > 500, what used to happen is that the error is displayed and the form submit button is disabled and the form stays visible. I cannot see in the diff where the refactoring of the error handling performs the disabling of the form submit button. Is this done somewhere and I have missed it? In any case, I don't think the relevant yui test checks for this so this should be added to the test. Or is this behaviour no longer relevant and we do something else? Can you explain it for me? Also, the original code, when if handled the 500 errors, set request_build_overlay=null to that the form would be recreated next time it was required (if this was not done, then the next time the form was opened it would be messed up). Is this still needed with the changes? Or is it also no longer required?

Thanks.

review: Needs Information (code*)

[Henning Eggers (henninge)]

Am 26.08.2011 03:12, schrieb Ian Booth:
> This is very cool. I really like the addition of the mock io stuff to
> the base lp client infrastructure; that's something I have been
> wanting to see and I'm really grateful you've done it.

I am really glad you like it. ;-)
I hope you noticed the previous branch which merged Aaron's iorecorder
code into mockio. See here:
https://code.launchpad.net/~henninge/launchpad/bug-824435-failure-reporting-2/+merge/72546
It was already landed but had to be rolled back because of the
instrumentation helpers not being included in launchpad.js, which I
fixed in thise branch

> have you considered updating the yui test page on the wiki?

Yes, I wanted to write somewhere about it, and about mockio, but did not
know where. Thanks for the tip.

>
> I cannot see in the diff where the refactoring of the error handling
> performs the disabling of the form submit button.

Line 525 of the diff. It is done right at the beginning of the function
that handles the form submission, so the button will be disabled in any
case.

> In any case, I don't think the relevant yui test checks for this so
> this should be added to the test.

It is not really part of what I am doing here but I admit it is part of
the code that I have added tests for here. I am happy to add that check
to the test.

> Also, the original code, when if handled the 500 errors, set
> request_build_overlay=null ... Or is it also no longer required?

That was a blunt hack to my eyes and only required because the code was
clobbering the form inside the formoverlay which made it impossible to
re-use it. Sine I am no longer doing the clobbering, there is no need
to have the form destroyed this way. Actually, I only refactored this at
this stage because it was messing up test isolation. I wanted to remove
the overlay markup from the page *and* delete the object but this code
made me loose the handle on the overlay.

Now, about the fact that this refactoring causes us to loose information
in case of a 5xx error I can point you to bug 824435 that this series of
branches is trying to fix (sorry, forgot to link the branch to it). The
ErrorHandler class in client.js has nice handling of 5xx errors which
extracts the OOPS id from the reply. The current handler in requestbuild
_overlay.js does not do this but simply returns a "Server error". The
request builds code solved this by displaying the full markup (including
traceback) in the overlay which I find a rather crude solution.
The next branch will fix this bug by including the OOPS id in the error
message because it will use the ErrorHandler from client.js. The OOPS
id is really all we need and this improved error message can be
displayed inside the overlay.

Hope to have been able to answer your questions. ;-)

[Ian Booth (wallyworld)]

Thanks for answering my questions here and on irc. As discussed, there seems to be an issue with the submit build button that's not part of the work here but which warrants a new bug. If a build request partially succeeds (eg 2 builds selected, one succeeds and one fails due to already building), the the user should be able to alter their selections to choose a different series to build against and resubmit without having to close and open the form again. I think that's how it used to work but there's been a lot of refactoring over a few branches and something may have got lost in the process.

review: Approve (code*)

[Jeroen T. Vermeulen (jtv)]

Well done both. Ian, I think this is your masterpiece, in the forgotten original sense: I see no reason why you should need further mentoring.

review: Approve (code)