1080122 - Need activity tests for Email<->Gallery and SMS<->Gallery to ensure that memory-backed blob issues don't come back

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[David Flanagan [:djf]]

Twice now, we've had bugs where Gallery's use of memory-backed blobs in activities (for cropped or rotated images) have caused crashes in the app at the other end of the activity.

The most recent bug was 1063658, and we landed a workaround to convert memory-backed blobs to file-backed blobs.  Hopefully bug 1079546 will fix this for good, but having been burned by this twice really shows that we need automated tests for this.

We ought to test that:

- Email and SMS (and maybe also homescreen) can pick images (including cropped and rotated image) from Gallery.

- Gallery can share images (including rotated images) with Email, SMS and the wallpaper app.

Even just testing a subset of these things for Email or SMS would be good.

I'm not sure how or where to write the necessary tests, however. The existing JS marionette tests for the SMS include a custom app to handle activities, which makes me think that maybe we don't want to introduce cross-app dependencies in those tests?

And the python marionette tests are fairly opaque to me and I don't know where to begin with those.

So basically I'll needinfo a bunch of people and see if anyone knows what to do and who can do it.

Comment 1

[David Flanagan [:djf]]

Needinfo Julien for SMS app.
Needinfo Andrew for the Email app.
Needinfo Punam and Russ because they know about the gallery app and testing
Needinfo James because he can probably tell us what to know and might know someone who could write the python tests

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

I think reasonable key goals here are:
- We want end-to-end testing
- We don't want to be at the mercy of other apps when testing.  Like it would suck for Gallery to be unable to land a patch because the "share" functionality test fails because the email app has gotten flakey/etc.

I like the SMS apps' approach here of having a dedicated app for testing, and think we can build on it to meet these goals.

I propose (and you may have seen me make a similar proposal before):
- We have a testing app for activities.
- Stored with the testing app are JSON files that indicate the expected request and response patterns.
- We test our apps against both sides.

So for example, the gallery app in its testing of share would:
- Use some helper logic to tell the test activity app which case is going to get run.
- Perform the share activity with the appropriate image files.
- The test activity app latches the received data and the marionette helper causes the payloads to be compared, including the contents of Blobs.
- The return value doesn't matter for share.

The email app in its corresponding testing of the same share test case would:
- Use helper logic to tell the test activity app to trigger the share activity with the specific payload and in a specific mode.  Because Blobs have been bad news for us, the share activity app would support different modes:
  - Share a memory-backed Blob
  - Share a DeviceStorage-backed File
  - Share an IndexedDB-backed File
- The email app does its tests based on that.


Key benefits:

- The "mastered" (like a record gets mastered) data files that live in a single place help define the authoritative contract honored by our apps.  If you want to change what your app is doing in terms of the payload, you need to change that file.  And the changes will immediately be felt by the other apps too.  There's no case of one app changing and subtly breaking other apps.

- This can benefit third-party apps by serving as both a source of documentation and something they can use in their own Marionette tests (to the extent that's possible).

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Oh, and let's avoid Python tests on this.

Comment 4

[Julien Wajsberg [:julienw]]

I think that having a dedicated fake app to test activities is very good because it's easier to trigger cases that maybe can't happen with the real apps, and we don't need to update them all in case an app changes its UI (yet all app interactions should be collected in a app-specific JS file anyway).

However, I think we need _some_ tests to run the whole stuff using all apps, maybe not triggering all cases, but at least the main case. Especially this case of sharing images is an important case and it's already the 2nd or 3rd time it's broken.

(Looping Oleg here, he wrote most of, if not all, the integration tests in SMS).

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> However, I think we need _some_ tests to run the whole stuff using all apps,
> maybe not triggering all cases, but at least the main case. Especially this
> case of sharing images is an important case and it's already the 2nd or 3rd
> time it's broken.

It's not clear to me that the extra realism buys us anything, while we have had serious problems with intermittently failing integration tests and slow integration tests (Gij takes 2 hours to run on gaia-try).

The reason this broke a bunch of times is that there was absolutely no test coverage and the problem might not have actually ever been totally fixed; both the original platform fix on bug 806503 and the subsequent platform fix on bug 982779 landed without any platform test coverage.  The removal of the gallery workaround in bug 1006919 that implied the fixes worked indicates some manual testing was done, but the STRs used to verify are not given so it's possible the STRs did not verify what was desired.  (For example, if the gallery app was left open during a pick activity so its process was not killed, etc.)  It's also possible there was a change in system app behaviour in terms of process termination at some point after that that caused a regression.

So my point is that I think the test helper app mechanism proposed absolutely gives us sufficient test coverage of these paths and it avoids us introducing redundant tests where there's a higher probability of intermittent failure and an unclear maintenance responsibility.  If we ever get a regression that the tests didn't catch, we should of course revisit that assumption.

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Andrew Sutherland [:asuth] from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #4)
> > However, I think we need _some_ tests to run the whole stuff using all apps,
> > maybe not triggering all cases, but at least the main case. Especially this
> > case of sharing images is an important case and it's already the 2nd or 3rd
> > time it's broken.
> 
> It's not clear to me that the extra realism buys us anything, while we have
> had serious problems with intermittently failing integration tests and slow
> integration tests (Gij takes 2 hours to run on gaia-try).

Gij should be split in several smaller tasks real soon now.

> 
> The reason this broke a bunch of times is that there was absolutely no test
> coverage and the problem might not have actually ever been totally fixed;
> both the original platform fix on bug 806503 and the subsequent platform fix
> on bug 982779 landed without any platform test coverage.  The removal of the
> gallery workaround in bug 1006919 that implied the fixes worked indicates
> some manual testing was done, but the STRs used to verify are not given so
> it's possible the STRs did not verify what was desired.  (For example, if
> the gallery app was left open during a pick activity so its process was not
> killed, etc.)  It's also possible there was a change in system app behaviour
> in terms of process termination at some point after that that caused a
> regression.
> 
> So my point is that I think the test helper app mechanism proposed
> absolutely gives us sufficient test coverage of these paths and it avoids us
> introducing redundant tests where there's a higher probability of
> intermittent failure and an unclear maintenance responsibility.  If we ever
> get a regression that the tests didn't catch, we should of course revisit
> that assumption.

Good enough for me.

Comment 7

[Russ Nicoletti [:russn]]

I like the idea of having a dedicated test app for testing sharing. It can provide more coverage than what we can achieve with app-to-app testing. Regarding Julien's comment about having some app to app tests, I think that's a good idea. Do we not have some in our smoke tests? If not, that's where I think they should go (if that's possible).

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think I added two decent platform-level tests for blob slicing stuff in bug 1079546 (though of course you should still continue here).

Comment 9

[Punam Dahiya [:pdahiya]]

Taking bug to add integration tests to validate blobs returned using pick and share gallery app activities.

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] punamdahiya:Bug1080122 > mozilla-b2g:master

Comment 11

[Punam Dahiya [:pdahiya]]

Comment on attachment 8593109 [details] [review]
[gaia] punamdahiya:Bug1080122 > mozilla-b2g:master

Hi David

Please review attached patch with marionette tests for pick and share activities in gallery app. As noted, these tests will help with testing and landing patch in bug 1080104. Thanks!

Comment 12

[Punam Dahiya [:pdahiya]]

Comment on attachment 8593109 [details] [review]
[gaia] punamdahiya:Bug1080122 > mozilla-b2g:master

Hi David

I am removing review flag on attached patch. I am almost done with gallery app open activity test and will submit the updated patch for your review soon.

Thanks!

Comment 13

[Punam Dahiya [:pdahiya]]

Comment on attachment 8593109 [details] [review]
[gaia] punamdahiya:Bug1080122 > mozilla-b2g:master

Hi David

I have updated attached patch to include pick, share and open activity test for gallery app. Please review. Thanks!

Comment 14

[David Flanagan [:djf]]

Comment on attachment 8593109 [details] [review]
[gaia] punamdahiya:Bug1080122 > mozilla-b2g:master

Punam,

I'm so happy to have these tests written. You've done a huge amount of work here. 

I've left a bunch of comments. Many are just questions and optional suggestions.  They fall into three basic categories:

1) requests for minor changes or comments or suggestions for other things you could test, if it is easy to make the changes.

2) suggestions about creating a lib/system.js library for the code that deals with the system app and the activity menu, so that you don't have to mix that stuff into lib/gallery.js

3) Concerns about timing issues. I want to be sure we don't get intermittent failures when running these tests if the system app is slow to launch an activity. I don't understand marionette well enough to know for sure whether you code has all of the waitFor() calls it needs to be sure that the activity handler app has launched before the test continues.

My r+ is contingent on you double-checking the timing stuff. Some of the other changes would be welcome, but if you're busy with other things, I'd be happy to have these tests landed even if you don't address the comments in categories 1 and 2.

Comment 15

[Punam Dahiya [:pdahiya]]

Thanks David for review! I will address your review comments and update patch.

Comment 16

[Punam Dahiya [:pdahiya]]

Hi David

I have updated the patch addressing comments in categories 1 and 2 and have double checked for any race conditions. I am setting flag for your feedback on the updated patch. Thanks!

Comment 17

[David Flanagan [:djf]]

Comment on attachment 8593109 [details] [review]
[gaia] punamdahiya:Bug1080122 > mozilla-b2g:master

Sorry it has taken me so long to get back to this, Punam. 

One question on github, but otherwise this looks good to me. Please go ahead and get it landed when you're ready.

Comment 18

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #17)
> Comment on attachment 8593109 [details] [review]
> [gaia] punamdahiya:Bug1080122 > mozilla-b2g:master
> 
> Sorry it has taken me so long to get back to this, Punam. 
> 
> One question on github, but otherwise this looks good to me. Please go ahead
> and get it landed when you're ready.

Thanks David! I addressed your question on github and updated patch. Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/e158dea4ff49514cacc42c3d7a9ac281779e41d7