1116566 - Create test to catch regression in bug 1116087

Status: RESOLVED FIXED

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

Description

[Peter Bylenga [:PBylenga]]

In Bug 1116087, closing and opening the email app creates two option bars breaking basic functionality of the app.

Comment 1

[Peter Bylenga [:PBylenga]]

NI on Bebe for assignment.

Comment 2

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

:mwargers, need to assign someone on this. Thanks!

Comment 3

[Martijn Wargers (dead)]

Attachment: regression_mail.diff

Something like this would test catch this regression.
I guess I need to make a new test file out of this, instead of adding stuff to test_setup_basic_gmail.py.
This isn't really part of a smoketest.

Comment 4

[Martijn Wargers (dead)]

Attachment: regression_mail

So like this? 
I know we normally don't put the locators in the test file, but in the app objects.
But this is testing a very specific thing and I don't think we would do such a thing more often.

Comment 5

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8543019 [details] [review]
regression_mail

To me, the main issue here is the test name which is not self-explanatory. If we include more tests with this kind of name, it would be a real pain to investigate a test run report.

Also, I think we can avoid to define a locator in the test by improving the architecture behind the Header page class. See more details in the PR.

Comment 6

[Martijn Wargers (dead)]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #5)
> To me, the main issue here is the test name which is not self-explanatory.
> If we include more tests with this kind of name, it would be a real pain to
> investigate a test run report.

For mochitests, this is standard practice, e.g. test_bug123456.html.
It's going to be quite difficult to keep having descriptive names for these kinds of things.
I'm not sure what to call this else: test_reopen_not_two_option_bars.py or something?

Comment 7

[Martijn Wargers (dead)]

Btw, in this case, it would be nice, if we would have a moch email server, so this test could then be run on treeherder. The whole manual email setup in this test is not really what we test, but is needed for the test, but that makes the test 'online = true' and hence, not runnable on treeherder.

Comment 8

[Johan Lorenzo [:jlorenzo]]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #6)
> For mochitests, this is standard practice, e.g. test_bug123456.html.
> It's going to be quite difficult to keep having descriptive names for these
> kinds of things.
Like we discussed offline, the OWD project used this kind of nomenclature for their own set of Gaia UI Tests[1]. They ended up writing a script[2] to sort the test names. To me, it's better to have an explicit names, especially when you want contributors to ramp up effectively.

> I'm not sure what to call this else: test_reopen_not_two_option_bars.py or
> something?
Sounds good. test_reopen_with_only_one_header.py could fit too.

[1] https://github.com/owdqa/owd_test_cases
[2] https://github.com/owdqa/OWD_TEST_TOOLKIT/blob/integration-v2.0/scripts/sort_test_descriptions.py

Comment 9

[Martijn Wargers (dead)]

Comment on attachment 8543019 [details] [review]
regression_mail

I updated the pull request with your suggestions.
Let me know what you think.
I updated the build on my phone to today's one and it passes with this test now.

Comment 10

[Martijn Wargers (dead)]

I kicked of a Jenkins adhoc run: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/531/

Comment 11

[Martijn Wargers (dead)]

Jenkins adhoc run is passing.

Comment 12

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8543019 [details] [review]
regression_mail

So we didn't break any email test with this refactor. The test looks good to me then.

Comment 13

[Viorela Ioia [:viorela]]

Comment on attachment 8543019 [details] [review]
regression_mail

Looks good, just a few comments to be addressed. 
Also, the test looks reliable, I started an adhoc and I got only one unrelated failure: http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/544/HTML_Report/

Comment 14

[Martijn Wargers (dead)]

Comment on attachment 8543019 [details] [review]
regression_mail

Updated pull request to address your comments.
For some reason in line 44 in (now called) test_email_one_header_displayed.py
, I had to add this:
self.email = Email(self.marionette)
Otherwise I got "JavascriptException: TypeError: appWindow is undefined" error when trying email.launch() the following line on b2g desktop.
I don't know why that happens now, the last time I tried, I didn't think I needed it. Oh, well.

Comment 15

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Resetting back to in-qa-testsuite? per decision this morning. Note that this is still not needed when adding the QA Whiteboard flags, which are generally more expressive.

Comment 16

[Martijn Wargers (dead)]

(In reply to Viorela Ioia [:viorela] from comment #13)
> Comment on attachment 8543019 [details] [review]
> regression_mail
> 
> Looks good, just a few comments to be addressed. 
> Also, the test looks reliable, I started an adhoc and I got only one
> unrelated failure:
> http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/544/HTML_Report/

Thanks, yeah, that failure I saw too. The email app doesn't open up properly somehow, sometimes. That's a bug in the email app, I think.

I followed your suggestion in the pull request, I added this in def close_app in cards_view.py.
+ if len(self.cards) == 0:
+ self.marionette.switch_to_frame()
+ self.wait_for_condition(lambda m: self.apps.displayed_app.name == Homescreen.name)

Let me know if you like this solution.

Comment 17

[Martijn Wargers (dead)]

Note that cards_view.py still has to be rewritten using the Wait() methods (bug 1074117), so I took the liberty of still using wait_for_condition here. It's easier to change it all at once for that file.

Comment 18

[Viorela Ioia [:viorela]]

Comment on attachment 8543019 [details] [review]
regression_mail

r+, but please address my last comment in the PR. Thanks!

Comment 19

[Martijn Wargers (dead)]

(In reply to Viorela Ioia [:viorela] from comment #18)
> Comment on attachment 8543019 [details] [review]
> regression_mail
> 
> r+, but please address my last comment in the PR. Thanks!

I added this as for your suggestion now (and removed the switch_to_frame call):
+ self.assertFalse(cards_view.is_app_displayed(self.email.name),
+ '%s app should not be present in cards view' % self.email.name)

I think this can be checked in now, right? (if you agree with this addition)

Comment 20

[Viorela Ioia [:viorela]]

Merged in: https://github.com/mozilla-b2g/gaia/commit/1e8725a92d35c06893a52d42eda34d1758ef3bb8