Closed Bug 1193978 Opened 7 years ago Closed 7 years ago

Automated Test [Clock] Use stopwatch, set a few laps, reset.

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Silne30, Assigned: Silne30)

References

Details

Attachments

(1 file)

Attachment #8647180 - Flags: review?(jlorenzo)
Attachment #8647180 - Flags: review?(martijn.martijn)
Comment on attachment 8647180 [details] [review]
[gaia] silne30:Bug_1193978_Stopwatch_Laps_Test > mozilla-b2g:master

The content of the tests looks good! I think we can improve the form a little bit more. See more details in the PR.
Attachment #8647180 - Flags: review?(jlorenzo)
Assignee: nobody → jdorlus
Comment on attachment 8647180 [details] [review]
[gaia] silne30:Bug_1193978_Stopwatch_Laps_Test > mozilla-b2g:master

Looks good. I made some comments in the pull request. Not sure if they are all valid, but some of them might be useful.
You need to add the new test files into the manifest.ini file.
Attachment #8647180 - Flags: review?(martijn.martijn)
I'm also wondering if we care if these tests would run correctly on b2g desktop, I haven't checked that. I guess we don't care anymore about that, since this is now hidden on Treeherder.
Attachment #8647180 - Flags: review?(jlorenzo)
Attachment #8647180 - Flags: review?(martijn.martijn)
Depends on: 1195676
Like in bug 1195305, the manifest hasn't been edited. These tests should belong in the dogfood suite.
Comment on attachment 8647180 [details] [review]
[gaia] silne30:Bug_1193978_Stopwatch_Laps_Test > mozilla-b2g:master

A couple of minor nits. The major issues to me are: The manifest missing and the verify_pause() in the page class. This function doesn't perform any verification (it helps it, though), which can confuse future readers.

More details in the PR.
Attachment #8647180 - Flags: review?(jlorenzo)
Comment on attachment 8647180 [details] [review]
[gaia] silne30:Bug_1193978_Stopwatch_Laps_Test > mozilla-b2g:master

I'll review (and mainly test) once you updated the pull request following the suggestions from Johan.
Attachment #8647180 - Flags: review?(martijn.martijn)
Attachment #8647180 - Flags: review?(jlorenzo)
Attachment #8647180 - Flags: review?(martijn.martijn)
The ad-hoc run that we did, passed completely. 

http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-kk.ui.adhoc/866/

I think we should be good to pull.
Comment on attachment 8647180 [details] [review]
[gaia] silne30:Bug_1193978_Stopwatch_Laps_Test > mozilla-b2g:master

Sorry, perhaps I'm totally mistaken, but as I understand it, verify_laps_not_same is wrong, because it's used in a situation where the stopwatch times should actually be the same, because the pause button was pressed.
Also, some other comments in the pull request.
Attachment #8647180 - Flags: review?(martijn.martijn) → review-
Attachment #8647180 - Flags: review?(martijn.martijn)
Comment on attachment 8647180 [details] [review]
[gaia] silne30:Bug_1193978_Stopwatch_Laps_Test > mozilla-b2g:master

Some more comments in the pull request. test_clock_run_stopwatch_laps.py didn't pass for me on b2g desktop.
Regarding test_clock_run_stopwatch_then_reset.py, I think the check surrounding the pause button is not useful. It doesn't check that the button works properly.
Attachment #8647180 - Flags: review?(martijn.martijn)
Side note: The adhoc job didn't start yesterday. Reason:
> There are no nodes with the label '' .

This means you need to provide a Jenkins slave node to execute the tests. Usually we use: "flame&&ui". This means you target a Flame device in the lab, that is attached to the UI jobs (and not a perf job for instance).

In order to get results when you awake. I started a new one http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/869/console
Attachment #8647180 - Flags: review?(martijn.martijn)
(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #11)
> Side note: The adhoc job didn't start yesterday. Reason:
> > There are no nodes with the label '' .
> 
> This means you need to provide a Jenkins slave node to execute the tests.
> Usually we use: "flame&&ui". This means you target a Flame device in the
> lab, that is attached to the UI jobs (and not a perf job for instance).
> 
> In order to get results when you awake. I started a new one
> http://jenkins1.qa.scl3.mozilla.com/job/flame-kk.ui.adhoc/869/console

I knew that may happen. Firefox Nightly blew away all of the fields before it crashed. I will do it again.
Attachment #8647180 - Flags: review?(martijn.martijn) → review+
Comment on attachment 8647180 [details] [review]
[gaia] silne30:Bug_1193978_Stopwatch_Laps_Test > mozilla-b2g:master

Looks good to me, modulo:
* https://github.com/mozilla-b2g/gaia/pull/31336/files?diff=split#r37523027
* https://github.com/mozilla-b2g/gaia/pull/31336/files?diff=split#r37557477
Attachment #8647180 - Flags: review?(jlorenzo)
Attachment #8647180 - Flags: review-
Attachment #8647180 - Flags: review+
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I just noticed this PR has never been merged. John, do you have the rights to merge this PR?
Status: RESOLVED → REOPENED
Flags: needinfo?(jdorlus)
Resolution: FIXED → ---
Merged this today. It had been so long that there were a ton of merge conflicts to fix. Done!
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Flags: needinfo?(jdorlus)
Resolution: --- → FIXED
Depends on: 1218930
Did you forget to squash the commits before merging?
Depends on: 1219344
John - please be more careful in the future. Every commit landed to gaia should contain a bug number, and be atmoic in nature, typically via squashing pull requests. I've gone ahead and backed out the PR and re-landed as a single commit.

Revert: https://github.com/mozilla-b2g/gaia/commit/d8c2ac6cb386be689bca5f84beecc68030f0a32b
Re-land: https://github.com/mozilla-b2g/gaia/commit/8a1b3d7c120b17838757c92226daadccf473c327
Flags: needinfo?(jdorlus)
Sorry about that. Noted.
Flags: needinfo?(jdorlus) → needinfo?(kevingrandon)
Flags: needinfo?(kevingrandon)
Depends on: 1225881
You need to log in before you can comment on or make changes to this bug.