add getProcessedData() for uploaded blobs

[Martin Pitt]

Right now it is a real pain to write test suites for Apport's launchpad backend, since the bug filing process has lots of AJAXy stuff in between which make it very brittle to automatically file a bug on staging. (It broke a few weeks ago, and I still don't have a real idea how to fix it).

It would be a lot easier if bugs could be filed through launchpadlib. For that, the createBug() method would need to grow an optional argument for specifying a +storeblob handle. This would also solve the problem that you need a valid HTML cookie for the test suite.

Thank you for considering!

Martin

Note: This bug was originally meant to add a new "blob ticket" argument to createBug(), but during the course of this bug something unrelated and different was implemented. The new bug for the original request is bug 604934.

[Gary Poster (gary)]

Apport is one of our most critical webservice users. Making it easier for apport to be more robust in this way seems like a high priority if it can be done reasonably quickly.

Since it would also help Martin make an apport test suite that LP could use for QA and maybe even automated tests, I'm even more interested.

Deryck, Leonard, is there a quick and reasonable way to expose adding a blob to a bug?

Thank you.

[Deryck Hodge (deryck)]

It would be trivial to make the createBug method accept a blob parameter. This would also allow apport to file bugs without going to the web UI permanently, assuming pitti likes this idea or wanted to do that.

This pushes dupe detection back to apport, but in some cases I would imagine apport would be better than Launchpad for that anyway. Otherwise, we could also expose the dupe detection code via the API.

Cheers,
deryck

[Bryce Harrington (bryce)]

Reading this bug there seems to be several actions needed:

1. Add a +storeblob parameter (is that actually the best variable name for this?)

2. Expose dupe detection from the API. [Is this already implemented via the bugTask.findSimilarBugs() method?]

(Fwiw, I 110% agree it'd be better for apport to control dupe settings, since apport hooks could do more sophisticated heuristics using package-specific hardware identification logic or comparing error messages or backtraces more directly. Functionality way too specialized to belong in Launchpad's codebase.)

3. Implement dupe detection in apport.

4. Write an automated test suite for apport's backend

5. Explore the idea of running apport's test suite as part of Launchpad's automated testing, to help prevent future regressions.

For this bug report, we'll focus just on item #1. Item #2 I'm going to assume can already be done using findSimilarBugs(); if that's not the case please file additional bug report(s) to clarify what issues remain there. Items #3 and #4 are on the apport side so will leave to Martin's capable hands. #5 probably deserves wider discussion beyond the scope of this bug report... I'll leave that to Deryck to follow up on as appropriate.

[Bryce Harrington (bryce)]

The code that unpacks and processes the blob data is in the browser side of the code. This functionality looks like it needs to be pushed down to the model layer. The blob contains a lot of data though and it's kind of interwoven amongst other logic. For example, you can pass multiple attachments, comments, subscribers, and other metadata in the blob.

Looking at the apport codebase, it looks like apport's code is roughly divided into one chunk which deals with attachments, and another chunk which deals with tags, privacy, subscribers, and other metadata. It isn't setting more than one message. It's not setting more than one subscriber either.

I wonder if it might make sense at the API layer to split things out and have some simpler calls. For instance, one call that *just* does attachments, and a separate one which receives arbitrary metadata as a dict.

[Martin Pitt (pitti)]

Bryce Harrington [2010-05-19 1:34 -0000]:
> 2. Expose dupe detection from the API. [Is this already implemented
> via the bugTask.findSimilarBugs() method?]
> 3. Implement dupe detection in apport.

This seems rather unrelated? Apport does not currently try to use
Launchpad's dupe detection, it just checks bug patterns and the
retracer uses the stack trace based dupe db.

> 4. Write an automated test suite for apport's backend

It already has one:

  python apport/crashdb_impl/launchpad.py -v

This currently uses screenscraping to open a bug through the +filebug
page, but it's getting more and more painful (and recently got
completely broken again due to changes in the openid authentication).

> 5. Explore the idea of running apport's test suite as part of
> Launchpad's automated testing, to help prevent future regressions.

Right, I discussed that with Diogo already, and I have some code
ready. But since the +filebug part is broken, it currently doesn't
work either. That's why this bug report is the missing piece.

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)

[Martin Pitt (pitti)]

Bryce Harrington [2010-05-19 3:27 -0000]:
> The code that unpacks and processes the blob data is in the browser side
> of the code. This functionality looks like it needs to be pushed down
> to the model layer.

Why? I want to continue to do the blob upload through http and the
+storeblob page. That works fine, since it doesn't need
authentication.

The thing I need is to move the bug object creation from
urlopen('https://.../+filebug?DEADBEEF') to the API, like
lp.createBug(..., blob_id='DEADBEEF').

This shouldn't need changes to the actual blob decoding in LP?

> The blob contains a lot of data though and it's kind of interwoven
> amongst other logic. For example, you can pass multiple
> attachments, comments, subscribers, and other metadata in the blob.

From what I know, you can only set private flag/subscribers/title in
the MIME header. The first part is the text comment, all other parts
are attachments.

> It isn't setting more than one message.

Confirmed.

> It's not setting more than one subscriber either.

It potentially can, though (and did so in the past). In particular,
the Mythbuntu guys are asking for this.

> I wonder if it might make sense at the API layer to split things out
> and have some simpler calls. For instance, one call that *just*
> does attachments, and a separate one which receives arbitrary
> metadata as a dict.

We already have API calls for adding comments and attachments, though.
I would like the test suite to use the exact same code path as the
actual bug filing, i. e. upload stuff through +storeblob and then
create a bug with the blob ticket number.

Thanks,

Martin
--
Martin Pitt | http://www.piware.de
Ubuntu Developer (www.ubuntu.com) | Debian Developer (www.debian.org)

[Bryce Harrington (bryce)]

Hi Martin, thanks for the clarification! I had assumed you were looking to move from use of the web interface to using launchpadlib calls, but I understand what you mean about using the web interface to avoid the LPL auth stuff. Seems there is more to solving this bug than initially thought...

We're still a bit unclear on this bug. Could you explain what exactly you're wanting to test here? I.e. are you testing that launchpad actually receives the data blob? Or something to do with the filebug interaction? We're wondering if we can just rely on launchpad's own tests to ensure the pages themselves are working correctly and just focus on testing the specific apport needs themselves.

[Martin Pitt (pitti)]

Bryce Harrington [2010-05-19 18:04 -0000]:
> Hi Martin, thanks for the clarification! I had assumed you were looking
> to move from use of the web interface to using launchpadlib calls

Not for the blob uploading, just for the bug filing. Or alternatively,
get a more robust way of using the +filebug page from a program (or,
any way that works at all at the moment..)

> We're still a bit unclear on this bug. Could you explain what exactly
> you're wanting to test here? I.e. are you testing that launchpad
> actually receives the data blob?

That, plus that it decodes the blob correctly, sets the right
subscribers and privacy status, etc. This is meant to become a part
of Launchpad's test suite for "important customers", as requested by
Diogo. I. e. we want to test that apport and launchpad have a working
interface to each other.

> We're wondering if we can just rely on launchpad's own tests to
> ensure the pages themselves are working correctly

If LP already has tests for blob uploading, decoding, and bug
attaching, then that's certainly sufficient.

> and just focus on testing the specific apport needs themselves.

I also use the test suite for apport's launchpad backend itself, to
both test new features, make sure that the existing ones keep working
through launchpad changes, and test my workarounds for Launchpad
bugs/crashes (of which we had and have plenty). For these tests I need
some "synthetic" bug reports which I can mess with during the tests.

[Bryce Harrington (bryce)]

Sorry martin, ignore those three screenshots, those were for a different bug report. :-P

By the way, good news on this bug... The new api call is getProcessedData(). I finished implementation + testing yesterday and got it reviewed and signed off by Graham. Launchpad is frozen for the 10.5 release right now, but once it's thawed I'll be landing it and you can start using it on edge. Guess that'll be some time next week.

[Martin Pitt (pitti)]

Bryce Harrington [2010-05-29 0:15 -0000]:
> By the way, good news on this bug... The new api call is
> getProcessedData(). I finished implementation + testing yesterday and
> got it reviewed and signed off by Graham.

Awesome, thanks Bryce!

Martin

[Ursula Junque (ursinha)]

Fixed in stable r10961 <http://bazaar.launchpad.net/~launchpad-pqm/launchpad/stable/revision/10961>

[Martin Pitt (pitti)]

Bryce,

ah, I noticed the new temporary_blob stuff in the "devel" API. However, I'm not yet sure how to use this. Say I have a blob ID 54321 which I got from the +storeblob page. Now I call createBug() etc. to get a new Bug() object, and set the user-visible description. I still fail to see where I would specify the 54321 ID during bug creation: getProcessedData() seems to give me the data from the blob, but what I'd like to test and get is the behaviour of the web UI +filebug/54321, which attaches the blob data to the newly created bug.

I guess I could do that manually with adding stuff to the description and attachments, but first that's part of what I want to test in LP instead of replicate on my side, and second this logic must already be in LP since it works for the web ui.

[Diogo Matsubara (matsubara)]

Set the foundations task to invalid since there is no need to update the webservice. The feature is available on edge and staging

[Martin Pitt (pitti)]

Reopening, see comment 15.

[Deryck Hodge (deryck)]

Let's don't change the milestone on this. The code has landed, it works well, and was rolled out in 10.07. There is some discussion that needs to happen about if this meets pitti's needs or not. I suspect it does, and we just need to demonstrate how it can be used. If this work doesn't meet pitti's needs, let's open a new bug about any additional work that needs to take place.

Graham was going to look at this with Bryce and then get in touch with pitti about how this might or might not work for him, but Graham is user-testing mockups today. He'll be in touch with Bryce and pitti Friday, I'm sure.

Cheers,
deryck

[Martin Pitt (pitti)]

Deryck Hodge [2010-07-08 14:25 -0000]:
> Let's don't change the milestone on this. The code has landed, it works
> well, and was rolled out in 10.07.

Right, it's just not doing what the bug description says (or at least
how I meant it), that's why I reopened it. If you prefer that I file a
new bug, that's fine for me.

[Graham Binns (gmb)]

Okay, so after reading the entire comment thread and then reading it again because I got confused, here's how I understand where we stand at the moment. Please correct any mistakes.

 1. We've added a method, getProcessedData(), to ITemporaryBlobStorage. This returns a dict containing the data that has been parsed from an uploaded Apport blob (i.e. the same data that the +filebug process uses to populate new bugs).
 2. The Launchpad test suite covers the correct parsing of Apport blobs (see http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/annotate/head:/lib/lp/bugs/tests/test_apportjob.py)
 3. The LP test suite covers using the data from an Apport blob in the +filebug process (see http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/annotate/head:/lib/lp/bugs/stories/guided-filebug/xx-bug-reporting-tools.txt)

Now, AIUI the use-case for this bug is the Apport test suite. Since it's been said above that Launchpad's own test coverage of blob parsing (assuming it's complete enough) is sufficient for the case of testing "apport uploads stuff and Launchpad inserts it in the bug appropriately, I'm not quite clear on why we still need to add a blob_id parameter to createBug(). Is there some test in the apport suite that requires an actual Bug to exist on the Launchpad side? At the moment it seems like we're heading for test duplication by actually creating a Bug in the Apport test suite. Especially since Apport never actually files bugs itself in real usage; it uploads the data and lets the user complete the report.

So, as I read it, I think we've solved the use-case for this bug, providing that we're happy that the LP test suite is comprehensive enough in its testing blobs being parsed and injected into the +filebug process. However, I admit to being a bit confused at this point, so feel free to tell me why I'm wrong.

[Graham Binns (gmb)]

Additionally, it's just occurred to me that testing LP's insertion of blob data into bugs in the Apport test suite is necessarily brittle, since we don't run the Apport test suite agains Launchpad as part of the LP QA process (maybe we should, but that's not the point I'm making).

[Gary Poster (gary)]

FWIW, yes, Diogo is leading a Foundations effort to run at least a subset of the Apport test suite against Launchpad as part of QA. That's supposed to lead to a pattern for webservice-based apps to be better incorporated into the release and QA process.

[Martin Pitt (pitti)]

Graham Binns [2010-07-09 13:48 -0000]:
> I'm not quite clear on why we still need to add a blob_id parameter to
> createBug(). Is there some test in the apport suite that requires an
> actual Bug to exist on the Launchpad side?

Right. The Apport launchpad backend test suite creates a few test bugs
on staging, reads them back, ensures that these multiple conversion
stages result in an identical data dictionary, and then tests
duplication, tagging, etc. I do this to ensure that all operations
performed by apport-collect and the retracer work as intended, and
that new functionality wrt. tagging bugs, changing the formatting
etc. do not break existing functionality.

Another important purpose of that test suite is to make sure that my
current workarounds for Launchpad/launchpadlib bugs still apply (or
conversely, to verify that they were fixed by removing the workarounds
and running the tests again).

> At the moment it seems like we're heading for test duplication by
> actually creating a Bug in the Apport test suite.

I need three bugs in a known state with known data which I can tamper
with in the test suite. What I'm interested in is doing operations on
those bugs. The actual bug creation is less interesting, but still
required to verify that my dict -> MIME -> blob -> bug attachments ->
parsing back into a dict process works.

> Especially since Apport never actually files bugs itself in real
> usage; it uploads the data and lets the user complete the report.

The client-side operation does the data -> MIME -> blob encoding part,
though.

Also, Diego keeps asking me to provide a full end-to-end client-side
test suite for Apport's operations, which can be integrated into
Launchpad's QA process. But with the OpenID changes introduced some
months ago (when I filed this bug) I'm unable to. Also, I am now
pretty hampered in doing bug fixes or changes to Apport's Launchpad
backend, since testing all those operations manually is a very time
consuming and error prone process.

[Martin Pitt (pitti)]

Graham Binns [2010-07-09 13:50 -0000]:
> Additionally, it's just occurred to me that testing LP's insertion of
> blob data into bugs in the Apport test suite is necessarily brittle,
> since we don't run the Apport test suite agains Launchpad as part of the
> LP QA process (maybe we should, but that's not the point I'm making).

Right, that's exactly what Diego was asking for.

[Martin Pitt (pitti)]

I cloned the original request into a new bug 604934.