1093874 - Create a new test case for loading a mix of valid/invalid image files into Gallery App

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Description

[No-Jun Park [:njpark]]

This bug is to automate https://moztrap.mozilla.org/manage/case/8617/ where it tests the loading of blank or empty image file.

In addition, this will also test loading of various types of valid/invalid files, which is known to cause db error in previous occasions.

Tests will use files listed here:
https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/test/images/README

Comment 1

[No-Jun Park [:njpark]]

since push_resource method takes remote directory path, files won't need to be copied to the resource folder I assume?

Comment 2

[No-Jun Park [:njpark]]

Since def resource(self, filename) in gaiatest.py explicitly looks for 'resource' folder, I'll copy the files into the resource folder as well.

Comment 3

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

My first attempt on submitting a new python script.  Could you take a look whether I'm on the right track?  Thanks!

Comment 4

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

New pull request for review

Comment 5

[No-Jun Park [:njpark]]

Comment on attachment 8517707 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

This is invalid, please ignore

Comment 6

[Zac C (:zac)]

Comment on attachment 8517833 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

r-,

There should be one test per file format.

Comment 7

[Zac C (:zac)]

Personally I don't think this is a really good use of automated testing, I think it will be very prone to false positives.

Comment 8

[No-Jun Park [:njpark]]

(In reply to Zac C (:zac) from comment #7)
> Personally I don't think this is a really good use of automated testing, I
> think it will be very prone to false positives.

So I was concerned about false positives too. (i.e., one of the invalid image can generate a thumbnail +  one of the valid images fail to generate a thumbnail = correct number of thumbnails)  The main reason I thought I could write a test on it was that when a bug happens,
1) invalid images usually cause gallery app to crash when loaded
2) invalid images usually create thumbnails, which upon tapping it, causes the app to crash
3) Decoding bugs usually fail to create thumbnails for valid images

It would been more robust if I tap all valid images and check for the back button, but that would mean I need to create a new method in the app that taps any of the thumbnails in the list, and wasn't sure I can do that.  I think this test could be a quick way to detect image decoding bugs when it happens.

Having said that, having a test script for every single case is sort of messy with a lot of overhead, and I could run this on imagecompare test framework to avoid false positive issue.  If you want me to drop this test case, I can close this bug and work on a different test.

Comment 9

[No-Jun Park [:njpark]]

actually, there is chance of false negatives, (saying there is no bug when there is), but I don't think there is a chance for false positive unless I missed something.  If this test fails, I think there would definitely be a problem, since all it does is a simple loading of images and checks for the number of thumbnails.

Comment 10

[Zac C (:zac)]

As long as you're clear about the limitations of what bugs it can and can't pick up wrt false positives and negatives.

Having more test cases is not messy; it means if one of the files fails, you do know which one it is. Having 8 checks in one means you don't know if only one failed or 5 failed so you have to run it again locally to find out which one it is which kind of takes away the advantage of running in a CI!

Comment 11

[No-Jun Park [:njpark]]

(In reply to Zac C (:zac) from comment #10)
> As long as you're clear about the limitations of what bugs it can and can't
> pick up wrt false positives and negatives.
> 
> Having more test cases is not messy; it means if one of the files fails, you
> do know which one it is. Having 8 checks in one means you don't know if only
> one failed or 5 failed so you have to run it again locally to find out which
> one it is which kind of takes away the advantage of running in a CI!

Hmm, that's a good point, I was only concerned about the overhead of running 8 test cases vs. 1, but I forgot about the fact that you have to run it again to make sure which one failed.  I'll update the test scrips.  thanks for the comment!

Comment 12

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

So I made changes to the pull request.  now it includes 14 test scripts, each takes slightly less than a minute to run.

7 added scripts are testing whether different file types can be opened by the gallery app.  I noticed that we only test opening of JPG files, and not other formats such as BMP, PNG, GIF, progressive JPG, animated JPG/PNG, and PNG with transparent background.  If the script exists and I missed it, I'll take them off.  verifying whether the app correctly loads above files is sort of important at this stage of the app, but we can always deactivate the tests.

The other 7 scripts are testing whether the gallery app correctly ignores the invalid file, rather than trying to create thumbnails for it and end up crashing.  Again, this can also be deactivated once the code matures.

I also made a change to the gallery app code, by adding a reference to the back button per comment.

Comment 13

[Zac C (:zac)]

Comment on attachment 8519088 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

r-:
- File missing from manifest files
- Too many abbreviations in test names (bytes are free, use them)
- Validate the image properly, not just the surrounding UI

After adding it to the manifest file, check that Gip is green before r? us again too!

Comment 14

[No-Jun Park [:njpark]]

Try server result:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=4fc2a7c19c84
(Gip is green)

- manifest adds new test script names
- abbreviations are removed
- now checks for display of the fullscreen view and image view to see the image is being displayed

Comment 15

[Zac C (:zac)]

Comment on attachment 8519088 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

r-, comments in github:

1. don't use private locator variables from in the test. You need to make a method in the app object that represents the action you're doing on the page. From inside that method you can call the private locator variables.

2. In my previous review what I meant about validating the image properly was to actually check the HTML element that is showing the image to the user. It must have a blob and a size, too.
It's no point checking the back button -  the gallery might fail to show the image completely but the back button will still be there!

Comment 16

[No-Jun Park [:njpark]]

Try server result:
https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=a9ab7b9f880f

There are some Gij failures, but I think this error is unrelated to my change.

I did:
- Added double_tap_image method in fullscreen_image.py, because this action zooms the image to its original size if the image isn't too big.  Let me know if I need to do this differently.
- checked for the presence of blob, and get the image dimension
- got rid of private var usage

One thing, while I was searching for examples, I ran into this:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/keyboard/app.py#L232

Although this method is not being used, this cannot work, because double_tap only belong to the Actions group. (I think) Just to let you know.

Comment 17

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

Re-adding the pull request.  previous one led to an invalid link

Comment 18

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

Comment 19

[No-Jun Park [:njpark]]

Comment on attachment 8523188 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

redirecting review per automation meeting discussion today

Comment 20

[Florin Strugariu [:Bebe]]

Comment on attachment 8523188 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

Let's try not to duplicate the test files
The rest looks OK

Comment 21

[Viorela Ioia [:viorela]]

Comment on attachment 8523188 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861

Comments in PR

Comment 22

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861/commits

@parameterized is used, and reduced the number of test cases.

Comment 23

[Florin Strugariu [:Bebe]]

Comment on attachment 8529866 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861/commits

Looking good
A few more nits and this is ready

Comment 24

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861/commits

setup call/comments removed

Comment 25

[Viorela Ioia [:viorela]]

Comment on attachment 8531686 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861/commits

There's one test failing :(

Comment 26

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861#issuecomment-66125678

commits squashed and Gip tests passed (one Gij failed, but it has nothing to do with my change)

Comment 27

[Viorela Ioia [:viorela]]

Comment on attachment 8533211 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861#issuecomment-66125678

r+

Comment 28

[Florin Strugariu [:Bebe]]

Comment on attachment 8533211 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861#issuecomment-66125678

No-Jun your commit message is really strange

https://github.com/npark-mozilla/gaia/commit/35364e00c72d0281ed223964c5e6b7ae0915dfb3


The rest looks OK

Can you add a commit message like "Bug 1093874 - Create a new test case for loading a mix of valid/invalid image files into Gallery App"

Comment 29

[Florin Strugariu [:Bebe]]

Comment on attachment 8531686 [details]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861/commits

Looks like there are to attachments to the same pull

marking as obsolete

Comment 30

[No-Jun Park [:njpark]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25861/commits

Thanks for pointing out the odd commit message.  here is the updated one

Comment 31

[Florin Strugariu [:Bebe]]

https://github.com/mozilla-b2g/gaia/commit/9e62614c692eb875e305dd7de4050880f1a17a2b