Closed Bug 1116566 Opened 5 years ago Closed 5 years ago

Create test to catch regression in bug 1116087

Categories

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

x86
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pbylenga, Assigned: martijn.martijn)

Details

Attachments

(1 file, 1 obsolete file)

In Bug 1116087, closing and opening the email app creates two option bars breaking basic functionality of the app.
NI on Bebe for assignment.
Flags: needinfo?(florin.strugariu)
:mwargers, need to assign someone on this. Thanks!
Flags: needinfo?(martijn.martijn)
QA Whiteboard: [fxosqa-auto-backlog?]
Assignee: nobody → martijn.martijn
QA Whiteboard: [fxosqa-auto-backlog?] → [fxosqa-auto-backlog+]
Flags: needinfo?(martijn.martijn)
Attached patch regression_mail.diff (obsolete) — Splinter Review
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.
Attached file 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.
Attachment #8542747 - Attachment is obsolete: true
Attachment #8543019 - Flags: review?(viorela.ioia)
Attachment #8543019 - Flags: review?(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.
Attachment #8543019 - Flags: review?(jlorenzo) → review-
(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?
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.
(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 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.
Flags: needinfo?(florin.strugariu)
Attachment #8543019 - Flags: review- → review?(jlorenzo)
Jenkins adhoc run is passing.
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.
Attachment #8543019 - Flags: review?(jlorenzo) → review+
QA Whiteboard: [fxosqa-auto-backlog+] → [fxosqa-auto-backlog+][fxosqa-auto-s7][fxosqa-auto-points=4]
Flags: in-qa-testsuite?(martijn.martijn)
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/
Attachment #8543019 - Flags: review?(viorela.ioia) → review-
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.
Attachment #8543019 - Flags: review- → review?(viorela.ioia)
Flags: in-qa-testsuite?(martijn.martijn) → in-qa-testsuite+
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.
Flags: in-qa-testsuite+ → in-qa-testsuite?
(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.
Flags: needinfo?(viorela.ioia)
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 on attachment 8543019 [details] [review]
regression_mail

r+, but please address my last comment in the PR. Thanks!
Flags: needinfo?(viorela.ioia)
Attachment #8543019 - Flags: review?(viorela.ioia) → review+
(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)
Flags: needinfo?(viorela.ioia)
Merged in: https://github.com/mozilla-b2g/gaia/commit/1e8725a92d35c06893a52d42eda34d1758ef3bb8
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(viorela.ioia)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.