986662 - Delete some redundant tests

Status: RESOLVED FIXED

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

Description

[Zac C (:zac)]

The following tests are not really giving us much value and we should nuke them and any app object methods that are no longer used.

test_browser_https_auth.py
test_calendar_today_date.py
test_clock_add_alarm_multiple_times.py
test_everythingme_search_accented.py
test_gallery_return_to_tile_view.py
test_persona_cookie.py
test_system_context_menu_activity_picker.py
test_edge_gestures.py
test_window_open_inside_iframe.py

Comment 1

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

(In reply to Zac C (:zac) from comment #0)
> The following tests are not really giving us much value and we should nuke
> them and any app object methods that are no longer used.

Please be cautious when removing any app object methods. These are not only used by the tests, but also by consumers of gaiatest such as eideticker, b2gperf, etc.

Comment 2

[Zac C (:zac)]

We won't remove anything obvious like `launch` but without any documented spec or the dependent tests in tree that'll always be a risk. (tbh I'm surprised we haven't hit this problem already!)

Comment 3

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

So you're saying you're going to delete stuff anyway and it's tough if something's using it? The app objects should offer methods to get/set state and interact with the application. Why would removing tests invalidate the related methods in the app objects?

Comment 4

[Zac C (:zac)]

Well we don't delete it if we know someone's using it but the best we can do to know if it's being used is to CC known consumers or search within the tree for its usage.

We're torn between the good ui testing practice of only keeping code that is used and just leaving it there 'just incase', or for it to rot?

Comment 5

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

(In reply to Zac C (:zac) from comment #4)
> Well we don't delete it if we know someone's using it but the best we can do
> to know if it's being used is to CC known consumers or search within the
> tree for its usage.
> 
> We're torn between the good ui testing practice of only keeping code that is
> used and just leaving it there 'just incase', or for it to rot?

I would keep anything that is still relevant to the app. It's true that it might rot if nothing is using it, but there's no easy way to check, and removing something is in use will have a very high impact and could be very frustrating. If there's anything that relates to functionality that's been removed from an app, or changed to the point that it no longer works, then I'm fine with removing it. Ideally all app object methods would be covered by unit tests, but I realise that's not likely to happen any time soon.

Comment 6

[Viorela Ioia [:viorela]]

Attachment: Github PR: https://github.com/mozilla-b2g/gaia/pull/17636

Comment 7

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

Comment on attachment 8397750 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/17636

The app object changes are minimal and my only concern here. If the tests are really no longer needed then I have no objection to them being removed. I don't believe any of the app object items deleted are used by any of our own packages that use gaiatest (but can't be certain), but also don't see the harm in leaving these in if they are still appropriate to the apps.

Comment 8

[Florin Strugariu [:Bebe]]

(In reply to Dave Hunt (:davehunt) from comment #7)
> Comment on attachment 8397750 [details] [review]
> Github PR: https://github.com/mozilla-b2g/gaia/pull/17636
> 
> The app object changes are minimal and my only concern here. If the tests
> are really no longer needed then I have no objection to them being removed.
> I don't believe any of the app object items deleted are used by any of our
> own packages that use gaiatest (but can't be certain), but also don't see
> the harm in leaving these in if they are still appropriate to the apps.


The pull looks OK to me and I don't think it's a good thing to leave unused methods in the app objects. They will rot and it will be harder to fix them in the future than reading them. 
To prevent breaking any other tests I would suggest sending a email to the gaia-UI mail list saying that we are removing these methods. 
If something goes wrong and we break other tests we can revert the commit.
Also can we have 2 commits in this pull 1 that deletes the tests and one that removes the app object methods.

Comment 9

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

(In reply to Florin Strugariu [:Bebe] from comment #8)
> (In reply to Dave Hunt (:davehunt) from comment #7)
> > Comment on attachment 8397750 [details] [review]
> > Github PR: https://github.com/mozilla-b2g/gaia/pull/17636
> > 
> > The app object changes are minimal and my only concern here. If the tests
> > are really no longer needed then I have no objection to them being removed.
> > I don't believe any of the app object items deleted are used by any of our
> > own packages that use gaiatest (but can't be certain), but also don't see
> > the harm in leaving these in if they are still appropriate to the apps.
> 
> 
> The pull looks OK to me and I don't think it's a good thing to leave unused
> methods in the app objects. They will rot and it will be harder to fix them
> in the future than reading them. 
> To prevent breaking any other tests I would suggest sending a email to the
> gaia-UI mail list saying that we are removing these methods. 
> If something goes wrong and we break other tests we can revert the commit.
> Also can we have 2 commits in this pull 1 that deletes the tests and one
> that removes the app object methods.

I disagree. We can't assume that everybody that installs gaiatest from PyPI is on the gaia-ui-automation mailing list. If modifying app objects breaks a consumer then the workflow of restoring that functionality is to first revert and then to release a new version of gaiatest. Add to this the likelyhood that the consumer first had to work out what happened and why their package suddenly stopped working, and it can be quite a disruptive and lengthy process. I'm speaking from experience as maintainer for three packages that depend on gaiatest.

There is certainly a risk of code rot, but one way to avoid that is to add unit tests for these methods, which could be selectively run. If we absolutely must remove these methods then they should be in separate commits (ideally split by app), and should probably follow after a duration of deprecation, so that we can warn consumers of the impending removal.

For the record, I do agree that we should remove unused code, but we can't assume this code is unused. Doing so will make gaiatest less useful to consumers and will likely push them to duplicate the code they need rather than contribute to making gaiatest a reusable package.

Comment 10

[Zac C (:zac)]

That would just be a total change of the way we've been working on this since it existed. If you take it to its literal level then we should be cautious of changing any app method at any time.  I know we've made changes to suit users in the past but they've always been very minor.

There's never been any rules or specification for how we manage these app methods and maintaining gaiatest. I also don't think it's within the ability (time-wise) to maintain it. The users don't contribute back into gaiatest significantly. Thus I think if you really want us to maintain it as you're suggesting then we do it either all in and we need to pitch to get more staff to help us maintain it in an orderly manner, create a doctrine for maintenance/service agreement, orderly timed releases, etc. Or we just keep doing it as we do, whereas we can only operate within reason of what's in master and asking the known consumer.

There is a level of protection in that the gaiatest version can be pinned by the user and updated on discretion.

I think the marketplace-tests-gaia sets a good precedent for how to use gaiatest; if it's not in gaiatest then it needs to be maintained in the repo locally. As such the whole marketplace app object was moved into the test repo, extending gaiatest. It's protected from changes in gaiatest and it can be tuned to suit the test repo's requirements.

Comment 11

[Zac C (:zac)]

Comment on attachment 8397750 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/17636


Not looking to overrule Dave's comments but just want to get perspective on whether anything in these app object is used.

Rob/Walter, can you let us know whether deletion of these app methods will affect you?

Cheers.

Comment 12

[Walter Chen[:ypwalter][:wachen]]

I don't think it will affect me. I can simply delete it.

Comment 13

[Robert Wood [:rwood]]

Comment on attachment 8397750 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/17636

I'm all for cleaning up or removing tests that are definitely unused/obsolete; it may be a bit risky removing app objects though...  however yep I had a look and the app objects in question aren't used by the gaia-ui endurance tests, so they wouldn't be affected. Thanks for checking first, appreciated!

Comment 14

[Walter Chen[:ypwalter][:wachen]]

Comment on attachment 8397750 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/17636

forgot to mark as +

Comment 15

[Zac C (:zac)]

Merged:
https://github.com/mozilla-b2g/gaia/commit/bac30d7f387233567bf85e4b11b111bcf7e3812c

Dave, Viorela wisely split into two commits the app object and the tests so in a pinch we can bring back the app object methods. I thought this was a good compromise.

Comment 16

[Zac C (:zac)]

Viorela can you do this for the v1.4 branch (but not v1.3)? Thanks

Comment 17

[Zac C (:zac)]

Comment on attachment 8397750 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/17636

r+

Comment 18

[Viorela Ioia [:viorela]]

Sure, I'll do it.

Comment 19

[Viorela Ioia [:viorela]]

Attachment: pull request for v1.4: https://github.com/mozilla-b2g/gaia/pull/18370

Comment 20

[Zac C (:zac)]

v1.4 adhoc - http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.hamachi.mozilla-aurora.ui.adhoc/48/

Comment 21

[Zac C (:zac)]

Comment on attachment 8407489 [details] [review]
pull request for v1.4: https://github.com/mozilla-b2g/gaia/pull/18370

r+ and it passed in the adhoc!

Comment 22

[Zac C (:zac)]

Merged:
https://github.com/mozilla-b2g/gaia/commit/0996eecb1b34e66dba6ab082a9b4f03e6861c0de