958835 - Add ignore_javascript_exceptions argument to launch method of gaiatest

Status: RESOLVED WONTFIX

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

Description

[Bob Silverberg [:bsilverberg]]

This is being added to address JavascriptExceptions being thrown during the tests for marketplace-tests-gaia. A switch will allow the caller to specify whether or not to ignore JavascriptExceptions that are thrown while launching an app.

Comment 1

[Bob Silverberg [:bsilverberg]]

Attachment: pull request adding ignore_javascript_exceptions option

Comment 2

[Zac C (:zac)]

Comment on attachment 8358941 [details] [review]
pull request adding ignore_javascript_exceptions option

r-, I don't think this should be in gaiatest, I don't think that gaiatest should take responsibility for this type of code.

I think we all agree it's a bit hacky and not a best practice, thus it should be kept in the test repo that uses it where the compromise is acceptable (in its context) Thus in future the marketplace owner has responsibility for it. I'm ok with using this technique in that repo, that's why I f+ it there and have r- it here.

I'm not concerned about code duplication; I'm concerned about bloating gaiatest with hacks like this that we will need to maintain in the future. 

This launch method is already bloated with code that isn't used/reached and this makes it worse.

I think gaiatest should be kept more like WebDriver became after Selenium where basic, lightweight features exist and the responsibility is on the user to use it safely, rather than the package itself bloating with features.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8358941 [details] [review]
pull request adding ignore_javascript_exceptions option

Could you try this with a Marionette Wait, which should reduce the code considerably? Unfortunately it seems this hack is necessary more often than we'd like, so I am fine with it being in gaiatest.

This means we can switch it on locally or even in-tree by changing the default boolean value, and anyone using gaiatest to test their own apps doesn't need to implement the hack themselves.

This would mean that any JavaScript exceptions caused by the app itself during launch will not be identified, but I think this is acceptable for now. Once we have Gaia UI tests running against the emulator we should be able to prevent these exceptions from making it into the nightly builds.

Comment 4

[Florin Strugariu [:Bebe]]

Comment on attachment 8358941 [details] [review]
pull request adding ignore_javascript_exceptions option

I agree with Dave that we should use Wait here and I agree with Zac that this method is not a good practice in gaiatest.

Adding this options would give us the opportunity to skip JS errors when we start apps. That's not a good practice and if we want to implement it for one project that's OK but we should not encourage this and our apps should not throw JS erros when they start.

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8358941 [details] [review]
pull request adding ignore_javascript_exceptions option

I updated the PR to use marionette's Wait. This really only adds a line of code to gaiatest (other than the workarounds I had to add until the next marionette_client is released), and I can see it being useful in other circumstances. To be honest, I did this work *before* reading the other comments on this bug, so I hadn't seen Zac's concerns yet.

I do still prefer this for the following reasons:
- it is only a tiny code change, versus the fairly complicated code required to do this in marketplace-tests-gaia
- although marketplace-tests-gaia is a smaller repo, it is still an important repo and we are going to have to maintain all of the code in it, so adding all that code does increase our maintenance cost. The argument that we don't want to have to maintain the code in gaia doesn't really make sense to me, as we're going to have to maintain the code somewhere in either case. So why not have less code to maintain?
- it doesn't affect the behaviour of gaiatest, unless a client specifically calls launch with the ignored_exceptions argument
- we may encounter other situations in which we need this

Comment 6

[Bob Silverberg [:bsilverberg]]

Ok, I take it all back. ;)

I was able to make the change to marketplace-tests-gaia with a lot less code this time, so although there is a bit duplicated from gaia, it's not such a big deal anymore.

I have a marketplace-tests-gaia-only solution now which I think addresses the issue better than this.

Comment 7

[Zac C (:zac)]

Dave actually I don't recall us needing this kind of code on launch before.
Where we do need it is in the startup where the script is executing for a long time thus exposed to any exceptions that occur during the startup.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #7)
> Dave actually I don't recall us needing this kind of code on launch before.
> Where we do need it is in the startup where the script is executing for a
> long time thus exposed to any exceptions that occur during the startup.

You're correct, I was mixing the two up. So this is a new issue when launching apps, or Marketplace specifically? If the latter it would suggest a Marketplace bug, otherwise this is a gaia bug and the fix (temporary hack) should be in gaia.

I couldn't find a bug for this, other than bug 958123 comment 0, which mentions @app://system.gaiamobile.org/js/cost_control.js. This would imply it's not a Marketplace only issue.