Closed Bug 1065933 Opened 10 years ago Closed 10 years ago

Marionette should throw a MarionetteException when a tap causes the frame to close unexpectedly

Categories

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

Other
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S7 (24Oct)

People

(Reporter: zcampbell, Assigned: davehunt)

References

Details

Attachments

(5 files, 4 obsolete files)

test_ftu_with_tour is crashing intermittently on Jenkins around 2/10 times.

We should try and capture the crash report and file it if it isn't already filed.

Jenkins failure:
http://jenkins1.qa.scl3.mozilla.com/job/flame.b2g-inbound.ui.functional.smoke/1421/testReport/test_ftu_with_tour/TestFtu/test_ftu_with_tour/
OS: Linux → Gonk (Firefox OS)
Priority: -- → P1
Hardware: x86_64 → Other
I ran this test over 20 times and I was unable to reproduce the issue
I've been able to reproduce this failure locally on latest master, by running the automated test several times.
It failed 2 times out of 21 runs, so it's kind of difficult to reproduce it.
I watched the phone while running the test, and I noticed that it passed through each step from FTU.
After tapping Let's go button, FTU app is closed and Homescreen is displayed for a few minutes. Then the test fails, and the phone restarts.
So there's no crash info displayed on the screen, in order to be able to get a crash report.
Logcat: https://pastebin.mozilla.org/

Traceback (most recent call last):
  File "/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/marionette_test.py", line 171, in run
    testMethod()
  File "/home/viorelaioia/WebQA/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/ftu/test_ftu_with_tour.py", line 41, in test_ftu_with_tour
    self.ftu.tap_lets_go_button()
  File "/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/gaiatest/apps/ftu/app.py", line 349, in tap_lets_go_button
    self.marionette.find_element(*self._lets_go_button_locator).tap()
  File "/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/marionette.py", line 90, in tap
    return self.marionette._send_message('singleTap', 'ok', id=self.id, x=x, y=y)
  File "/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/decorators.py", line 35, in _
    return func(*args, **kwargs)
  File "/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/marionette.py", line 621, in _send_message
    "Connection timed out", status=errors.ErrorCodes.TIMEOUT)

TEST-INFO took 430326ms
test_end for test_ftu_with_tour.py TestFtu.test_ftu_with_tour logged while not in progress. Logged with data: {"status": "ERROR", "extra": {}, "expected": "PASS", "test": "test_ftu_with_tour.py TestFtu.test_ftu_with_tour", "message": "MarionetteException: MarionetteException: Please start a session\n\n", "stack": "Traceback (most recent call last):\n  File \"/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/marionette_test.py\", line 199, in run\n    self.tearDown()\n  File \"/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/gaiatest/gaia_test.py\", line 944, in tearDown\n    MarionetteTestCase.tearDown(self)\n  File \"/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/marionette_test.py\", line 354, in tearDown\n    self.marionette.set_context(\"content\")\n  File \"/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/marionette.py\", line 949, in set_context\n    return self._send_message('setContext', 'ok', value=context)\n  File \"/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/decorators.py\", line 35, in _\n    return func(*args, **kwargs)\n  File \"/home/viorelaioia/.virtualenvs/gaia_moz/lib/python2.7/site-packages/marionette_client-0.8.3-py2.7.egg/marionette/marionette.py\", line 606, in _send_message\n    raise errors.MarionetteException(\"Please start a session\")\n"}

Gaia      6cb5e0100d70313e4922c8d34bf20dcdd66ef616
Gecko     https://hg.mozilla.org/mozilla-central/rev/0be3ea11a4c7
BuildID   20140911160248
Version   35.0a1
ro.build.date   Mon Jun 16 16:51:29 CST 2014
ro.bootloader   L1TC00011220
ro.build.version.incremental   109
It sounds like a race between switching the frame and the find, or maybe the find starts, then the frame changes and then marionette dies.
Attached file logcat (obsolete) —
logcat of the issue
I can replicate this (on b2g-i builds) at a very high frequency, around 5/10, but having a very hard time narrowing down what causes it.

It does seem that Marionette loses its attachment to the frame when performing the tap.

If I change the tap() to a click(), I get a different albeit probably related message and 100% repetition. In this case Marionette does not hang; the test fails, but the suite will continue onto the next test briskly.


TEST-UNEXPECTED-ERROR | test_ftu_with_tour.py TestFtu.test_ftu_with_tour | MarionetteException: MarionetteException: The frame closed during the click, recovering to allow further communications


Traceback (most recent call last):
  File "/home/zac/.virtualenvs/gaiatest/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/marionette_test.py", line 264, in run
    testMethod()
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/ftu/test_ftu_with_tour.py", line 43, in test_ftu_with_tour
    self.ftu.tap_lets_go_button()
  File "/home/zac/Mozilla/gaia/tests/python/gaia-ui-tests/gaiatest/apps/ftu/app.py", line 353, in tap_lets_go_button
    fin.click()
  File "/home/zac/.virtualenvs/gaiatest/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/marionette.py", line 79, in click
    return self.marionette._send_message('clickElement', 'ok', id=self.id)
  File "/home/zac/.virtualenvs/gaiatest/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/decorators.py", line 35, in _
    return func(*args, **kwargs)
  File "/home/zac/.virtualenvs/gaiatest/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/marionette.py", line 638, in _send_message
    self._handle_error(response)
  File "/home/zac/.virtualenvs/gaiatest/local/lib/python2.7/site-packages/marionette_client-0.8.4-py2.7.egg/marionette/marionette.py", line 712, in _handle_error
    raise errors.MarionetteException(message=message, status=status, stacktrace=stacktrace)

TEST-INFO took 70720ms



Malini, we had this kind of error when the frame closed out from underneath Marionette far in the past but now it seems to have come back. Have you any idea about how/why this bug might be occurring?
Flags: needinfo?(mdas)
From the first logcat, the b2g process itself crashed, and in the second logcat, Marionette is successfully able to send a message to the underlying frame, so that's not the issue. I do see E/GeckoConsole( 9441): [JavaScript Error: "uncaught exception: Error getting previous version: "] following the last command to marionette, so may be that's what's triggering an error here.
Flags: needinfo?(mdas)
The 2nd logcat here isn't the one we're after, it's the first one we need to fix.
Attachment #8489981 - Attachment is obsolete: true
Attached file ftu_crash.log
Here is a logcat that doesn't include a crash.

The last few dozen lines are it starting the next test (as I had the test run set to repeat the test numerous times).

There is a 6 minute gap between the tap and the next test starting which corresponds with the Marionette hang/timeout.
Attachment #8488603 - Attachment is obsolete: true
The last two logcats had a bunch of hardware composer failures after running the test_ftu_with_tour test (and the composer restarts after). Seems like that is causing the failure. 

2982 09-18 16:40:55.247 I/ServiceManager(  191): service 'android.security.keystore' died
2983 09-18 16:40:55.247 I/ServiceManager(  191): service 'media.resource_manager' died
2984 09-18 16:40:55.247 I/ServiceManager(  191): service 'SurfaceFlinger' died
2985 09-18 16:40:55.247 I/ServiceManager(  191): service 'permission' died
2986 09-18 16:40:55.247 I/ServiceManager(  191): service 'display.qservice' died

Does this crash the ftu app? If the composer kills the ftu app, maybe there's a case where we don't see NS_ERROR_NOT_INITIALIZED in the logs which would be interesting for https://bugzilla.mozilla.org/show_bug.cgi?id=984508#c4
It's hard to tell whether it kills the FTU app or not because the very tap it's executing is intended to close the app anyway. Perhaps it does crash and die momentarily before it should be closed normally, but the timing is indistinguishable to the human eye.
NB - unfortunately we cannot get a good regression range on this because it started failing when we had that 1 week outage with the big crasher.
I think we need to check with some manual testing whether the FTU is crashing when we tap 'Let's go' or closing cleanly.
Aha, if the tap is causing the application to close or crash, then it's likely that marionette isn't recovering. I thought we had this recovery but it turns out that we only added a recovery process for 'click' but not for touch events (https://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#1692). We must add a similar handler for touch events. I'll need to look closer at this, but don't have the time just yet. I can take a look next week.
Flags: needinfo?(mdas)
Started seeing a similar error for test_launch_everything_me_link in today's v2.2 test run.
http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-319.mozilla-central.ui.functional.smoke/167/HTML_Report/

The test would time out after we tap on the e.me result icon.
I was able to reproduce the issue locally with automation 5 out of 5.
test_ftu_with_tour has also been failing frequently on Jenkins.

Build info:
Gaia-Rev        e1fd99454b6cd5da4f2c58f928fc04c6d03f478f
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/f4037194394e
Build-ID        20140922160202
Version         35.0a1
Device-Name     flame
FW-Release      4.3
FW-Incremental  110
FW-Date         Fri Jun 27 15:57:58 CST 2014
Bootloader      L1TC00011230
Robert, can you raise the e.me issue as a separate bug? It sounds like a crasher and it's definitely a different STR, we might need to get that to the attention of different people.
Flags: needinfo?(robert.chira)
I do not believe the error is caused by a crash. From the screenshot and local testing I can say that the tap seems to work. The link from e.me is launched successfully and the browser can be interacted with.
Flags: needinfo?(robert.chira)
This patch makes marionette recover when the frame it's touching closes. When a tap, action or multiaction causes the frame to close, Marionette will now switch back to the top most frame (system app in this case) and will throw an error saying: "The frame closed during the <action type>, recovering to allow further communications". If you know your action will close a frame, like in this test, it should catch this error and continue.

will r? when tries run green:

Mnw:
https://tbpl.mozilla.org/?tree=Try&rev=6c7cddbd9d00

Mn:
 https://tbpl.mozilla.org/?tree=Try&rev=1320ed4bae31
Flags: needinfo?(mdas)
Comment on attachment 8494723 [details] [diff] [review]
add a listener for mozbrowserclose

try is green for Mn and Mnw
Attachment #8494723 - Flags: review?(jgriffin)
I've seen test_ftu_skip_tour.py failing in the latest v2.1 build: http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-kk-319.mozilla-aurora.ui.functional.smoke/2/HTML_Report/
I suppose it's because of the issue described in this bug, as the test behaves the same when it's failing. 
Also, I can see the failures described in comment 10 in the logcat of the test failure.
The fix here should be uplifted to v2.1

Device firmware (date) 	25 Sep 2014 04:30:34
Device firmware (incremental) 	eng.cltbld.20140925.073024
Device firmware (release) 	4.4.2
Device identifier 	flame
Gaia date 	25 Sep 2014 00:29:46
Gaia revision 	c5d2e2f4ebf5
Gecko build 	20140925040206
Gecko revision 	32acbe1d64dc
Gecko version 	35.0a1
Comment on attachment 8494723 [details] [diff] [review]
add a listener for mozbrowserclose

Review of attachment 8494723 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.
Attachment #8494723 - Flags: review?(jgriffin) → review+
pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2559d4d07e99

Marking as [leave open] so once this lands, we will verify if this solves the problem
Whiteboard: [leave-open]
Hah! That's because this patch throws an exception when the frame is closed in test_ft_with_tourpy, instead of [silently failing|randomly working].

Zac, can the test_ftu_with_tour.py test be deactivated until this patch lands? We can then modify test_ftu_with_tour.py to expect the "frame closed" exception and continue, then turn it back on.
Flags: needinfo?(mdas) → needinfo?(zcampbell)
I don't really understand; there are numerous other scenarios when we close frames without raising exceptions and indeed this test has worked for at least a year until recently when Marionette started getting tripped up.

In the test we intend to close the frame so why should we be protecting against an exception?
Flags: needinfo?(zcampbell)
(In reply to Zac C (:zac) from comment #25)
> I don't really understand; there are numerous other scenarios when we close
> frames without raising exceptions and indeed this test has worked for at
> least a year until recently when Marionette started getting tripped up.
> 
> In the test we intend to close the frame so why should we be protecting
> against an exception?

I'm guessing it worked via race condition, where the tap closed the frame after marionette returned a response to the client. If a tap closes the frame before marionette could send a response, then you'll see this time out, because the frame marionette was operating within died, and we had no mechanism to recover. I don't know how it could have worked otherwise, because there was no recovery process for this case. We did have this recovery mechanism for click() though, so if the test used click, then it would respond with an exception. Which tests did this work for?

We throw an exception because, from the perspective of marionette, having a frame close during execution of an action is unexpected behaviour. What happens in this case is the currently active frame disappears and the main marionette-server instance (living in main gecko process) receives a message that the frame closed. Our only option here is to return some message to the client, telling them that the frame they were previously operating in was closed.

If you prefer, instead of throwing an exception, we could let the client use a command to set a flag, and if that flag is true, we will send back an Ok instead of an Exception. Maybe something like close_button.tap(expectFrameClose=True)?
I just don't see how the test framework should have to deal with whether Marionette has encountered a problem or not. As far as the test framework is concerned we just want to execute the tap and that's that. If something goes wrong with the underlying app we'll detect it on the next step because we'd be in the wrong frame (ie we'd get a NO_SUCH_ELEMENT_EXCEPTION). Capturing or ignoring the exception in the framework side is all this will be doing anyway; ignoring Marionette's problem. It's not a Gaia problem because Gaia is still usable to a human, only not usable to Marionette. I can't think of any scenarios where we would want to know when the frame has switched out from underneath Marionette yet the AUT is still usable.

IMO Marionette should just handle it silently, fall back to the system frame but without the session/port getting jammed. Of course this means I think we should remove the exception from the click too!

I did have a look in the WD spec but I couldn't find anything. I'd like to know what AutoamtedTester thinks.
Flags: needinfo?(dburns)
Originally the idea behind throwing an error was so that people know that they are being ejected from the frame and placed in the top most frame. Since we do this when an event is firing if we have a situation where the app crashed the frame we would be ejected and you as a user would get weird and wonderful error messages like "Element not found".

Having the exception means that you have some form of determinism in these situations. If you know that you are going to be ejected, and note this is only for b2g specific frames (because they are *special*), you can easily wrap the code in a try/except but if you get ejected without warning your test will fail with a meaningful exception.

This won't be in the spec because it is Mozilla specific.
Flags: needinfo?(dburns)
Right Ok, go ahead with this patch then. If we take it as is (and patch the test framework) then we don't need to release a marionette client either which is easier.

We'll have to get the team to land the test patch before this as I'm off today (or I can do it tomorrow). Bebe/Rob/Viorela, can one of you add the catch exception into the FTU app object for this test?
Flags: needinfo?(viorela.ioia)
Flags: needinfo?(robert.chira)
Flags: needinfo?(florin.strugariu)
Assignee: nobody → robert.chira
Flags: needinfo?(robert.chira)
Flags: needinfo?(viorela.ioia)
Robert started to work on this
Flags: needinfo?(florin.strugariu)
The test is currently disabled on master. (It's not failing on 2.1)
Malini, did your patch land in marionette? What kind of exception should we be trying to catch in our test framework?
Flags: needinfo?(mdas)
(In reply to Robert Chira [:RobertC] from comment #31)
> The test is currently disabled on master. (It's not failing on 2.1)
> Malini, did your patch land in marionette?What kind of exception should we
> be trying to catch in our test framework?

I'll land this today.

You'll be looking for a MarionetteError (code 500) with the text "The frame closed during the click, recovering to allow further communications".
Flags: needinfo?(mdas)
Well, I'll check it in soon, m-i is closed.
(In reply to Robert Chira [:RobertC] from comment #31)
> The test is currently disabled on master. (It's not failing on 2.1)
> Malini, did your patch land in marionette? What kind of exception should we
> be trying to catch in our test framework?

The test isn't disabled for tbpl. It needs to be disabled here: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/tbpl-manifest.ini#L62. Can you disable it? I'll reland then.
Flags: needinfo?(mdas)
Attached file PR_Added_Exceptions_In_Framework (obsolete) —
Attachment #8497319 - Flags: review?(zcampbell)
Attachment #8497319 - Flags: review?(viorela.ioia)
Attachment #8497319 - Flags: review?(florin.strugariu)
Comment on attachment 8497319 [details] [review]
PR_Added_Exceptions_In_Framework

r-, we don't want to assert the exception, we just want to `pass` when the Exception occurs.

Marionette cannot guarantee that it will raise this exception. It's an intermittent issue based on the timing/speed of Gaia.

Anyway let's hold off on this patch until we can also enable the test on TBPL at the same time, then we can be sure that this and Malini's patch is working on Gaia-Try.
Attachment #8497319 - Flags: review?(zcampbell)
Attachment #8497319 - Flags: review?(viorela.ioia)
Attachment #8497319 - Flags: review?(florin.strugariu)
Attachment #8497319 - Flags: review-
I believe we should assert the error code and message. We should not pass all the exceptions that might occur at that step. Also, the assertions are done in the except block and will only run when an exception is raised.
(In reply to Robert Chira [:RobertC] from comment #39)
> I believe we should assert the error code and message. We should not pass
> all the exceptions that might occur at that step. Also, the assertions are
> done in the except block and will only run when an exception is raised.

We're not testing Marionette, we're testing Gaia.

Already this test is intermittent and sometimes it has this issue and sometimes it doesn't. If you assert the exception then it will just intermittently fail on the assertion and sometimes it will not.
Comment on attachment 8497319 [details] [review]
PR_Added_Exceptions_In_Framework

I updated the PR, will ask for review after Malini's patch lands.
Attachment #8497319 - Flags: review-
I can't land this patch until the test is disabled on tbpl: https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/tbpl-manifest.ini#L62. Disable it, and then I'll land the patch.
The test is disabled. 
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/tbpl-manifest.ini#L63

I checked the last tbpl run and the test is marked as skipped.
Flags: needinfo?(mdas)
Summary: Capture crash report for intermittent crash in test_ftu_with_tour → Marionette should throw a MarionetteException when a tap causes the frame to close unexpectedly
Malini, we're seeing the exception raised by this patch on some tap actions that don't close frames. Testing on Flame.

I've worked around it by adding in a sleep(1) immediately after the tap which is suggesting to me that there's some kind of race condition between the tap and this mozbrowserclose event. 

Can you shed any more light on why this is happening and should we back this out?
Flags: needinfo?(mdas)
I updated the PR by adding try/catch in all the places I am currently seeing the exception thrown.
Attachment #8497319 - Attachment is obsolete: true
Attachment #8500386 - Flags: review?(zcampbell)
Attachment #8500386 - Flags: review?(florin.strugariu)
Attachment #8500386 - Flags: feedback?(mdas)
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

We don't want to do anything until mdas has responded to her ni?

There is something really wrong with the marionette code because, as demonstrated by your pull request, it is raising Exceptions when it shouldn't and switching back to the system frame, from which we need to switch back into the AUT.
When using these apps before mdas's patch a tap never caused marionette to fall back to the system frame.
Attachment #8500386 - Flags: review?(zcampbell)
Attachment #8500386 - Flags: review?(florin.strugariu)
Attachment #8500386 - Flags: feedback?(mdas)
This problem is because some other frame is crashing and emitting the mozbrowserclose event.

This patch keeps track of the id of the remote iframe we care about. If we get a mozbrowserclose message from it, only then will we throw this exception.

I've tested the ftu and settings tests with this patch and it looks happy. Running on try:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3f0e80230d4c
Attachment #8501269 - Flags: review?(jgriffin)
Flags: needinfo?(mdas)
Comment on attachment 8501269 [details] [diff] [review]
verify the sender of mozbrowserclose message

Review of attachment 8501269 [details] [diff] [review]:
-----------------------------------------------------------------

Nice fix!
Attachment #8501269 - Flags: review?(jgriffin) → review+
The patch has landed on m-c. How's this build looking?
Flags: needinfo?(zcampbell)
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

The build looks good.
I updated the PR to catch the exceptions.
Attachment #8500386 - Flags: review?(viorela.ioia)
Attachment #8500386 - Flags: review?(florin.strugariu)
Flags: needinfo?(zcampbell)
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

r+
Attachment #8500386 - Flags: review?(viorela.ioia) → review+
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

r-, just catch the exception and pass.

No need to validate the exception.
Attachment #8500386 - Flags: review-
Malini, would it be possible to update the exception with a more specific error code (not 500, the default one)? That way we can make the try/catch more exact without having to assert error messages.
Flags: needinfo?(mdas)
(In reply to Robert Chira [:RobertC] from comment #57)
> Malini, would it be possible to update the exception with a more specific
> error code (not 500, the default one)? That way we can make the try/catch
> more exact without having to assert error messages.

This is doable, while I'm at it, are there other exceptions you'd like to have numbered?

On a related note, webdriver will be getting rid of error codes, but I like them because that way we don't need to do string matching... dburns, for Bug 1059995, would the future way be to check the string? Could we also include an error code, just for marionette?
Flags: needinfo?(mdas) → needinfo?(dburns)
I was wondering whether this could be a:
FRAME_SEND_FAILURE_ERROR = 55

The name sounds like what might happen if the Frame goes away during an action, but I can't find a definition for this Exception. It isn't a WebDriver standard exception.
(In reply to Zac C (:zac) from comment #59)
> I was wondering whether this could be a:
> FRAME_SEND_FAILURE_ERROR = 55
> 
> The name sounds like what might happen if the Frame goes away during an
> action, but I can't find a definition for this Exception. It isn't a
> WebDriver standard exception.

Nice find! This is a custom exception so it's not part of WebDriver. I'll update the patch
I just changed the 500 error code to 55 in addFrameCloseListener
Attachment #8501269 - Attachment is obsolete: true
Attachment #8503286 - Flags: review?(jgriffin)
Attachment #8503286 - Flags: review?(jgriffin) → review+
Taking this as Malini is not around today. This patch applies cleanly and just includes the error code change. Carrying r+ from jgriffin.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d2e3817a853c
Assignee: robert.chira → dave.hunt
Status: NEW → ASSIGNED
Attachment #8504022 - Flags: review+
Dave, in the Try build there I see you intended to run "gaia-ui-tests" but I can't see any Gip jobs have run. Is it just exceptionally slow or was something not set correctly?
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

I know catching a exception and not validating it it's  bad code but I somehow agree with zac here

I think we over complicate stuff here
Attachment #8500386 - Flags: review?(florin.strugariu) → review-
(In reply to Zac C (:zac) from comment #63)
> Dave, in the Try build there I see you intended to run "gaia-ui-tests" but I
> can't see any Gip jobs have run. Is it just exceptionally slow or was
> something not set correctly?

I just reran the same syntax as Malini. I'm not sure why the gaia ui tests didn't run.
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

Disregard my previous comment.

I did not noticed this pull has to be updated
Attachment #8500386 - Flags: review- → review?(florin.strugariu)
(In reply to Dave Hunt (:davehunt) from comment #65)
> (In reply to Zac C (:zac) from comment #63)
> > Dave, in the Try build there I see you intended to run "gaia-ui-tests" but I
> > can't see any Gip jobs have run. Is it just exceptionally slow or was
> > something not set correctly?
> 
> I just reran the same syntax as Malini. I'm not sure why the gaia ui tests
> didn't run.

OK sure, I don't think that should block this anyway. We're not changing functionality in this patch.


Setting checkin-needed on the patch in comment #62.

(Kind of confusing having two patches going in one bug here)
Keywords: checkin-needed
(In reply to Malini Das [:mdas] from comment #58)
> (In reply to Robert Chira [:RobertC] from comment #57)
> > Malini, would it be possible to update the exception with a more specific
> > error code (not 500, the default one)? That way we can make the try/catch
> > more exact without having to assert error messages.
> 
> This is doable, while I'm at it, are there other exceptions you'd like to
> have numbered?
> 
> On a related note, webdriver will be getting rid of error codes, but I like
> them because that way we don't need to do string matching... dburns, for Bug
> 1059995, would the future way be to check the string? Could we also include
> an error code, just for marionette?

Instead of magic numbers we will return a string and then include a message like we currently do. For history this was suggested by Hixie and no one thought that it was a bad idea. It means we dont have to worry about making sure we have enough buffer between numbers.

What benefit would we have by keeping numbers over strings? Happy to discuss on IRC and dump response afterwards if that would be better :)
Flags: needinfo?(dburns) → needinfo?(mdas)
(In reply to Zac C (:zac) from comment #67)
> (In reply to Dave Hunt (:davehunt) from comment #65)
> > (In reply to Zac C (:zac) from comment #63)
> > > Dave, in the Try build there I see you intended to run "gaia-ui-tests" but I
> > > can't see any Gip jobs have run. Is it just exceptionally slow or was
> > > something not set correctly?
> > 
> > I just reran the same syntax as Malini. I'm not sure why the gaia ui tests
> > didn't run.
> 
> OK sure, I don't think that should block this anyway. We're not changing
> functionality in this patch.
> 
> 
> Setting checkin-needed on the patch in comment #62.
> 
> (Kind of confusing having two patches going in one bug here)

https://hg.mozilla.org/integration/b2g-inbound/rev/06adb3903dba
Keywords: checkin-needed
Target Milestone: --- → 2.1 S7 (24Oct)
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

r-, let's get the actual exception rather than checking the code.
Attachment #8500386 - Flags: review?(florin.strugariu)
Attachment #8500386 - Flags: review+
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

I updated the PR, but Malini's patch has yet to reach m-c. I tested it on b2g-inbound
Attachment #8500386 - Flags: review- → review?(zcampbell)
test change updated, thanks Robert :)
https://github.com/mozilla-b2g/gaia/commit/841d0d7d1b879f0ff4b5a8727f5dd23c7b0000a9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 8500386 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/24826

r+
Attachment #8500386 - Flags: review?(zcampbell) → review+
This issue from comment 0 started appearing on v2.1.
Can we get an uplift of the patches there please?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Issue was reproduced on flame v2.1 KK Full Flash:
Device firmware (date) 	16 Oct 2014 00:43:30
Device firmware (incremental) 	eng.cltbld.20141016.034320
Device firmware (release) 	4.4.2
Device identifier 	flame
Gaia date 	15 Oct 2014 14:08:45
Gaia revision 	477a9e61c3ed
Gecko build 	20141016001201
Gecko revision 	67573e422a0f
Gecko version
moved discussion about error codes to https://bugzilla.mozilla.org/show_bug.cgi?id=1059995
Flags: needinfo?(mdas)
Malini, can we get your patches uplifted to v2.1?
Flags: needinfo?(mdas)
QA Whiteboard: [fxosqa-auto-backlog+]
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
QA Whiteboard: [fxosqa-auto-backlog+] → [fxosqa-auto-s1]
You need to log in before you can comment on or make changes to this bug.