1139083 - Implement Brick Verification Test in Python

Status: RESOLVED FIXED

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

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] silne30:master > mozilla-b2g:master

Comment 2

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8572278 [details] [review]
[gaia] silne30:master > mozilla-b2g:master

This looks like a good start but there's an issue which can make this test act like a false positive. More details in the PR.

Apart from that, I wonder if we should have this test in the functional folder and if so, if we need an entry in the homescreen manifest to express the fact that's a super-sanity test.

Comment 3

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

Comment on attachment 8572278 [details] [review]
[gaia] silne30:master > mozilla-b2g:master

John, since you already have a minus and we've talked a bit in IRC, looking for an update.

Comment 4

[John Dorlus [:Silne30]]

This has been updated and has new commits. Changing from r- to r?. Thanks guys.

Comment 5

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8572278 [details] [review]
[gaia] silne30:master > mozilla-b2g:master

r+ modulo the nits that should be cleaned up before merging it in master:
* https://github.com/mozilla-b2g/gaia/pull/28613/files#r25776829
* https://github.com/mozilla-b2g/gaia/pull/28613/files#r26059113

Comment 6

[John Dorlus [:Silne30]]

The nits have been made.

Comment 7

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

Comment on attachment 8572278 [details] [review]
[gaia] silne30:master > mozilla-b2g:master

I asked for two minor changes (revert .gitignore and a method name change) but otherwise think this looks great.

I am a little concerned because Johan left a comment you said you fixed, but usually github marks the comment as obsolete when you update the pull request. Please doublecheck that the PR is fully up to date.

Those done, r+ from me. Just add checkin-needed to keywords once the PR is fully up to date, and Autolander will merge it.

Comment 8

[Johan Lorenzo [:jlorenzo]]

(In reply to Geo Mealer [:geo] from comment #7)
> I am a little concerned because Johan left a comment you said you fixed
It was probably related to another comment I left. In any case, you're right, it was a naming issue.

> Those done, r+ from me. Just add checkin-needed to keywords once the PR is
> fully up to date, and Autolander will merge it.
Also, before adding checking-needed, please squash your commits into one[1] and change the commit message to "Bug 1139083 - Implement Brick Verification Test in PythonBug. r=geo".

[1] Example: https://wiki.mozilla.org/Web_Testing/Automation/CodeReviewProcess#Squashing_commits_before_merging_a_feature_branch

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] silne30:Impliment_Brick_Test > mozilla-b2g:master

Comment 10

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

Comment on attachment 8578950 [details] [review]
[gaia] silne30:Impliment_Brick_Test > mozilla-b2g:master

Afraid there are some issues here, noted in the PR. Some I should have caught in the last review, and very sorry about that.

Comment 11

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

Comment on attachment 8578950 [details] [review]
[gaia] silne30:Impliment_Brick_Test > mozilla-b2g:master

John, based on your most recent update, I'm flipping to r+.

However, before you check in, I've asked that wait_for_homescreen_visible() be a function, not a property. Properties should only be used for simple accessors, and this function does a possibly significant wait and doesn't return anything.

I've echoed that in the PR.

You don't need another review from me after doing that. With the PR updated for that change (and commits squashed) you can merge on Johan's r+.

Comment 12

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8572278 [details] [review]
[gaia] silne30:master > mozilla-b2g:master

Agreed with Geo's comment. Also, if I understand the manifest correctly, this test would be run on Treeherder. One issue is that gaia-try-hook didn't pick your new PR (but it did on the previous one) so we're not able to check the result of gaia-try.

Before merging your code, I would contact a sheriff to make sure we won't make the new test permafail on Treeherder. This can prevent you from being ping'd by a sheriff and backing out your commit.

Comment 13

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8578950 [details] [review]
[gaia] silne30:Impliment_Brick_Test > mozilla-b2g:master

Meh, I commented on the wrong PR.

Comment 14

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#fxjYIl6TR2yPvH1mydhFSQ

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 15

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#ASeWi2ltQx-N-JZs5-cOWg

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 16

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/28933

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.

Comment 17

[GitHub Autolander Bot]

https://github.com/mozilla-b2g/gaia/pull/28933

Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.

Comment 18

[GitHub Autolander Bot]

http://docs.taskcluster.net/tools/task-graph-inspector/#6dcVAyofSEW_-n7bvN5ilg

The pull request failed to pass integration tests. It could not be landed, please try again.

Comment 19

[John Dorlus [:Silne30]]

I merged this one myself. This has been completed. TreeHerder is still failing this merge based on tests that were not affected by this change.