Closed Bug 1139083 Opened 5 years ago Closed 5 years ago

Implement Brick Verification Test in Python

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Silne30, Assigned: Silne30)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → jdorlus
QA Whiteboard: [fxosqa-auto-s11+]
Summary: Implement Super Sanity Test in Python → Implement Brick Verifcation Test in Python
Attachment #8572278 - Flags: superreview?(jlorenzo)
Attachment #8572278 - Flags: review?(gmealer)
Attachment #8572278 - Flags: superreview?(jlorenzo) → review?(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.
Attachment #8572278 - Flags: review?(jlorenzo) → review-
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.
Attachment #8572278 - Flags: review?(gmealer) → review-
Attachment #8572278 - Flags: review?(jlorenzo)
Attachment #8572278 - Flags: review?(gmealer)
Attachment #8572278 - Flags: review-
This has been updated and has new commits. Changing from r- to r?. Thanks guys.
Status: NEW → ASSIGNED
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
Attachment #8572278 - Flags: review?(jlorenzo) → review+
The nits have been made.
QA Whiteboard: [fxosqa-auto-s11+] → [fxosqa-auto-s11+][fxosqa-auto-s12]
QA Whiteboard: [fxosqa-auto-s11+][fxosqa-auto-s12] → [fxosqa-auto-from-s11+][fxosqa-auto-s12]
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.
Attachment #8572278 - Flags: review?(gmealer) → review+
(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
Attachment #8578950 - Flags: review?(gmealer)
Attachment #8578950 - Flags: review?(jlorenzo)
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.
Attachment #8578950 - Flags: review?(gmealer) → review-
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+.
Attachment #8578950 - Flags: review- → review+
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 on attachment 8578950 [details] [review]
[gaia] silne30:Impliment_Brick_Test > mozilla-b2g:master

Meh, I commented on the wrong PR.
Attachment #8578950 - Flags: review?(jlorenzo) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
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.
Keywords: checkin-needed
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.
Keywords: checkin-needed
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.
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.
Summary: Implement Brick Verifcation Test in Python → Implement Brick Verification Test in Python
Keywords: checkin-needed
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.
QA Whiteboard: [fxosqa-auto-from-s11+][fxosqa-auto-s12] → [fxosqa-auto-from-s11+, s12, s13]
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.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.