cron-job-ui-screenshots #12423

isabelrios · 2020-07-09T10:51:46Z

The aim of this PR is to set a cron job so that the ui screenshots tests run daily/weekly to be sure they are not broken and so, ready to be run when needed to get the app screenshots requested.

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

isabelrios · 2020-07-09T10:52:38Z

.cron.yml

@@ -32,3 +32,9 @@ jobs:
          treeherder-symbol: bump-ac
          target-tasks-method: bump_android_components
      when: [{hour: 14, minute: 0}]
+    - name: ui-test-screenshots-x86
+      job:
+          type: decision-task


@AaronMT not sure if this has to be the type...since there is no need for this task to appear in the decission tasks list for each PR/merge

Not sure (@rpappalax ?)

This is the right type for this use case. There is another supported value, for instance: https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/.cron.yml#303. That said, I had to search deeply to make sure there was another type. I don't know the story behind it.

isabelrios · 2020-07-09T10:53:33Z

.cron.yml

+          type: decision-task
+          treeherder-symbol: ui-test-screenshots-x86-D
+          target-tasks-method: ui-test-screenshots-x86
+      when: [{hour: 4, minute: 0}]


I guess this is to run the task daily at that time, is it possible to set the run to once a week for example? I don't think is necessary to run them daily...

https://fossies.org/linux/firefox/taskcluster/docs/cron.rst

e.g, Mondays and Thursdays at 10 AM

when: - {weekday: 'Monday', hour: 10, minute: 0} - {weekday: 'Thursday', hour: 10, minute: 0}

isabelrios · 2020-07-09T10:54:16Z

taskcluster/ci/ui-test/kind.yml

+                - [wget, {artifact-reference: '<signing-android-test/public/build/noarch/geckoNightly/target.apk>'}, '-O', android-test.apk]
+                - [automation/taskcluster/androidTest/ui-test.sh, x86-screenshots-tests, app-x86.apk, android-test.apk, '-1']
+
+            secrets:


I think we could move part of this to the job-defaults...

'-1' (shard override) to the script not supported anymore on latest Flank -> 50 (?) or anything more than -1

fenix/taskcluster/ci/ui-test/kind.yml

Line 31 in 8b34f37

- [automation/taskcluster/androidTest/ui-test.sh, x86, app.apk, android-test.apk, '50']

I think we could move part of this to the job-defaults...

I agree! As far as I can see, most of the job has common values with x86-debug. Since these are the only 2 jobs, I think we can factorize every common bit into job-defaults.

isabelrios · 2020-07-09T14:37:42Z

taskcluster/ci/ui-test/kind.yml

@@ -52,3 +52,42 @@ jobs:
                  path: /builds/worker/artifacts
                  type: directory
        worker-type: b-android
+    ui-test-screenshots-x86:


@AaronMT @rpappalax so that this task does not run as a build task and so be a build blocker, should I create a new folder in taskcluster/ci/ui-test-screenshots/ and put there the kind.yml for this?

Here is great! We put job that have about the same values in the same kind. We're in this case here, so let's keep this ui-test-screenshots-x86 in this kind.yml 🙂

Nit: Rename ui-test-screenshots-x86 into screenshots-x86. I just realized the name of the task looks odd otherwise: https://firefox-ci-tc.services.mozilla.com/tasks/Rbj7yALeQy--ZhDx6kMmcw

JohanLorenzo

This patch is definitely headed in the right direction! 😃 There are a couple more things to make it work. Good news is: you can test your patch out. I left some explanations down below. That said, I know the documentation is partial so feel free to ask me if you have any questions!

I'm happy to review this patch later to make sure it lands safe and sound!

JohanLorenzo · 2020-07-13T13:55:58Z

.cron.yml

@@ -32,3 +32,9 @@ jobs:
          treeherder-symbol: bump-ac
          target-tasks-method: bump_android_components
      when: [{hour: 14, minute: 0}]
+    - name: ui-test-screenshots-x86
+      job:
+          type: decision-task


This is the right type for this use case. There is another supported value, for instance: https://searchfox.org/mozilla-central/rev/af5cff556a9482d7aa2267a82f2ccfaf18e797e9/.cron.yml#303. That said, I had to search deeply to make sure there was another type. I don't know the story behind it.

JohanLorenzo · 2020-07-13T14:00:43Z

taskcluster/ci/ui-test/kind.yml

+                - [wget, {artifact-reference: '<signing-android-test/public/build/noarch/geckoNightly/target.apk>'}, '-O', android-test.apk]
+                - [automation/taskcluster/androidTest/ui-test.sh, x86-screenshots-tests, app-x86.apk, android-test.apk, '-1']
+
+            secrets:


I think we could move part of this to the job-defaults...

I agree! As far as I can see, most of the job has common values with x86-debug. Since these are the only 2 jobs, I think we can factorize every common bit into job-defaults.

JohanLorenzo · 2020-07-13T14:01:43Z

taskcluster/ci/ui-test/kind.yml

@@ -52,3 +52,42 @@ jobs:
                  path: /builds/worker/artifacts
                  type: directory
        worker-type: b-android
+    ui-test-screenshots-x86:


Here is great! We put job that have about the same values in the same kind. We're in this case here, so let's keep this ui-test-screenshots-x86 in this kind.yml 🙂

JohanLorenzo · 2020-07-13T14:02:31Z

taskcluster/ci/ui-test/kind.yml

+            signing-android-test: signing-android-test-debug
+        description: Run UI screenshots tests to keep them up to date
+        include-pull-request-number: true
+        #run-on-tasks-for: [github-pull-request, github-push]


Nit: Let's be explicit that we don't run this job in regular graphs.

Suggested change

#run-on-tasks-for: [github-pull-request, github-push]

run-on-tasks-for: []

JohanLorenzo · 2020-07-13T14:18:15Z

.cron.yml

+      job:
+          type: decision-task
+          treeherder-symbol: ui-test-screenshots-x86-D
+          target-tasks-method: ui-test-screenshots-x86


You need to a new entry in target_tasks.py, just like this one:

fenix/taskcluster/fenix_taskgraph/target_tasks.py

Lines 68 to 75 in 4624bf5

@_target_task("bump_android_components")

def target_tasks_bump_android_components(full_task_graph, parameters, graph_config):

"""Select the set of tasks required to update android components."""

def filter(task, parameters):

return task.attributes.get("bump-type", "") == "android-components"

return [l for l, t in full_task_graph.tasks.iteritems() if filter(t, parameters)]

I recommend something like:

@_target_task("screenshots") def target_tasks_screnshots(full_task_graph, parameters, graph_config): """Select the set of tasks required to generate screenshots on a real devive.""" def filter(task, parameters): return task.attributes.get("screenshots", False) return [l for l, t in full_task_graph.tasks.iteritems() if filter(t, parameters)]

Speaking of which, I don't think this task needs to have a long name. We may for instance want to generate screenshots on another platform. In this case, everything can happen in the same graph, which means it gets scheduled by the same decision task. So I think we can name it screenshots:

Suggested change

target-tasks-method: ui-test-screenshots-x86

target-tasks-method: screenshots

What do you think?

By the way, you can test this change, by applying https://github.com/mozilla-mobile/shared-docs/blob/master/android/taskcluster_guide.md#how-do-i-run-a-staging-nightlyrelease-on-prs. In this case, try_task_config.json should look like:

{ "parameters": { "optimize_target_tasks": true, "target_tasks_method": "screenshots" }, "version": 2 }

yeah! I can rename it :) only here or that name has to be consistent also in the kind.yml file too?

No need to change kind.yml. target-task-method just applies to target_tasks.py

JohanLorenzo · 2020-07-13T16:08:59Z

taskcluster/ci/ui-test/kind.yml

@@ -52,3 +52,42 @@ jobs:
                  path: /builds/worker/artifacts
                  type: directory
        worker-type: b-android
+    ui-test-screenshots-x86:
+        attributes:
+            build-type: debug


TODO: add

screenshots: true

isabelrios · 2020-07-14T09:30:48Z

@JohanLorenzo I did the changes requested, how does it look like now?
I know I have to remove the file I added in the root directory try_task_config.json :) will do that before merging but in case I have to try new things to fix, I will keep it until final review

JohanLorenzo

This patch looks great to me! I looked at the Taskcluster part and I just see a few nits. The job looks to be correctly executed, which is awesome!

I think we want to expose screenshots, which can easily be done by putting them in /builds/worker/artifacts/. This has to be done in the shell script, which I don't know really much. I leave that part of the review to a QA person 🙂

JohanLorenzo · 2020-07-15T08:42:11Z

taskcluster/ci/ui-test/kind.yml

@@ -52,3 +52,42 @@ jobs:
                  path: /builds/worker/artifacts
                  type: directory
        worker-type: b-android
+    ui-test-screenshots-x86:


Nit: Rename ui-test-screenshots-x86 into screenshots-x86. I just realized the name of the task looks odd otherwise: https://firefox-ci-tc.services.mozilla.com/tasks/Rbj7yALeQy--ZhDx6kMmcw

JohanLorenzo · 2020-07-15T08:45:30Z

taskcluster/ci/ui-test/kind.yml

+                - [automation/taskcluster/androidTest/ui-test.sh, x86-screenshots-tests, app.apk, android-test.apk, '-1']
+        treeherder:
+            kind: test
+            platform: 'ui-test-screenshots-x86/opt'


Nit: Let's keep the same platform as the regular UI-tests. Per se, those tests do run on the same platform. We can easily know which one is the screenshot job thanks to the TH symbol.

JohanLorenzo · 2020-07-15T08:47:31Z

taskcluster/ci/ui-test/kind.yml

+        treeherder:
+            kind: test
+            platform: 'ui-test-screenshots-x86/opt'
+            symbol: debug(ui-test-screenshots-x86)


Nit: Let's make this name simpler by just naming it screenshots-x86. We might want to bake "x86: into the TH platform, but it can be done in a followup.

JohanLorenzo · 2020-07-15T08:47:53Z

taskcluster/ci/ui-test/kind.yml

+            kind: test
+            platform: 'ui-test-screenshots-x86/opt'
+            symbol: debug(ui-test-screenshots-x86)
+            tier: 2


Nit: This can be factorized in job-defaults

JohanLorenzo · 2020-07-15T08:48:20Z

taskcluster/fenix_taskgraph/target_tasks.py

+
+@_target_task("screenshots")
+def target_tasks_screnshots(full_task_graph, parameters, graph_config):
+    """Select the set of tasks required to generate screenshots on a real devive."""


Nit:

Suggested change

"""Select the set of tasks required to generate screenshots on a real devive."""

"""Select the set of tasks required to generate screenshots on a real device."""

JohanLorenzo · 2020-07-15T08:51:12Z

automation/taskcluster/androidTest/ui-test.sh

@@ -80,6 +80,8 @@ elif [[ "${device_type}" == "x86-start-test" ]]; then
    flank_template="${PATH_TEST}/flank-x86-start-test.yml"
 elif [[ "${device_type}" == "arm-start-test" ]]; then
    flank_template="${PATH_TEST}/flank-armeabi-v7a-start-test.yml"
+elif [[ "${device_type}" == "x86-screenshots-tests" ]]; then


⚠️ Screenshots aren't exposed on Taskcluster. This script needs to be updated to download them and move them to /builds/worker/artifacts/

Actually this task does not get the screenshots, it only runs the tests. What we could expose there is the test results?

Cool, then it seems we're all done, I see some test results publihsed https://firefox-ci-tc.services.mozilla.com/tasks/Rbj7yALeQy--ZhDx6kMmcw#artifacts 😃

isabelrios · 2020-07-16T08:05:19Z

@JohanLorenzo I think I did all the requested changes 😅

codecov-commenter · 2020-07-16T10:28:45Z

Codecov Report

Merging #12423 into master will increase coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #12423      +/-   ##
============================================
+ Coverage     24.95%   24.98%   +0.03%     
  Complexity      836      836              
============================================
  Files           389      389              
  Lines         15657    15657              
  Branches       2022     2022              
============================================
+ Hits           3907     3912       +5     
+ Misses        11439    11432       -7     
- Partials        311      313       +2

Impacted Files	Coverage Δ	Complexity Δ
.../fenix/browser/browsingmode/BrowsingModeManager.kt	`85.71% <0.00%> (-14.29%)`	`0.00% <0.00%> (ø%)`
...mponents/searchengine/FenixSearchEngineProvider.kt	`60.36% <0.00%> (+1.80%)`	`14.00% <0.00%> (ø%)`
...nix/components/toolbar/BrowserToolbarController.kt	`69.64% <0.00%> (+2.38%)`	`0.00% <0.00%> (ø%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af3c232...6d42e3f. Read the comment docs.

JohanLorenzo · 2020-07-16T14:51:03Z

The force hook will be added in https://phabricator.services.mozilla.com/D83791. In the meantime, I made it available at https://firefox-ci-tc.services.mozilla.com/hooks/project-releng/cron-task-mozilla-mobile-fenix%2Fscreenshots, but this link may disappear next time ci-admin runs

isabelrios commented Jul 9, 2020

View reviewed changes

isabelrios added the pr:do-not-land label Jul 9, 2020

isabelrios commented Jul 9, 2020

View reviewed changes

isabelrios mentioned this pull request Jul 9, 2020

Cron job for screenshots tests mozilla-mobile/mobile-test-eng#59

Closed

JohanLorenzo reviewed Jul 13, 2020

View reviewed changes

isabelrios requested a review from JohanLorenzo July 14, 2020 09:28

JohanLorenzo approved these changes Jul 15, 2020

View reviewed changes

cron-job-ui-screenshots

6d42e3f

isabelrios force-pushed the ui-screenshots-tests-add-cron-job branch from c23b614 to 6d42e3f Compare July 16, 2020 09:24

JohanLorenzo merged commit 5b61b44 into mozilla-mobile:master Jul 16, 2020

liuche mentioned this pull request Jul 20, 2020

Releng Firefox Beta 79.1.0 #12761

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cron-job-ui-screenshots #12423

cron-job-ui-screenshots #12423

isabelrios commented Jul 9, 2020

isabelrios Jul 9, 2020 •

edited

Loading

AaronMT Jul 9, 2020

JohanLorenzo Jul 13, 2020

isabelrios Jul 9, 2020

AaronMT Jul 9, 2020

isabelrios Jul 9, 2020

AaronMT Jul 9, 2020 •

edited

Loading

JohanLorenzo Jul 13, 2020

isabelrios Jul 9, 2020

JohanLorenzo Jul 13, 2020

JohanLorenzo Jul 15, 2020

JohanLorenzo left a comment

JohanLorenzo Jul 13, 2020

JohanLorenzo Jul 13, 2020

JohanLorenzo Jul 13, 2020

JohanLorenzo Jul 13, 2020

JohanLorenzo Jul 13, 2020

isabelrios Jul 13, 2020

JohanLorenzo Jul 13, 2020

JohanLorenzo Jul 13, 2020

isabelrios commented Jul 14, 2020

JohanLorenzo left a comment

JohanLorenzo Jul 15, 2020

JohanLorenzo Jul 15, 2020

JohanLorenzo Jul 15, 2020

JohanLorenzo Jul 15, 2020

JohanLorenzo Jul 15, 2020

JohanLorenzo Jul 15, 2020

isabelrios Jul 15, 2020

JohanLorenzo Jul 15, 2020

isabelrios commented Jul 16, 2020

codecov-commenter commented Jul 16, 2020 •

edited

Loading

JohanLorenzo commented Jul 16, 2020

	#run-on-tasks-for: [github-pull-request, github-push]
	run-on-tasks-for: []

	@_target_task("bump_android_components")
	def target_tasks_bump_android_components(full_task_graph, parameters, graph_config):
	"""Select the set of tasks required to update android components."""

	def filter(task, parameters):
	return task.attributes.get("bump-type", "") == "android-components"

	return [l for l, t in full_task_graph.tasks.iteritems() if filter(t, parameters)]

	target-tasks-method: ui-test-screenshots-x86
	target-tasks-method: screenshots

	"""Select the set of tasks required to generate screenshots on a real devive."""
	"""Select the set of tasks required to generate screenshots on a real device."""

cron-job-ui-screenshots #12423

cron-job-ui-screenshots #12423

Conversation

isabelrios commented Jul 9, 2020

Pull Request checklist

After merge

To download an APK when reviewing a PR:

isabelrios Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronMT Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanLorenzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelrios commented Jul 14, 2020

JohanLorenzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isabelrios commented Jul 16, 2020

codecov-commenter commented Jul 16, 2020 • edited Loading

Codecov Report

JohanLorenzo commented Jul 16, 2020

isabelrios Jul 9, 2020 •

edited

Loading

AaronMT Jul 9, 2020 •

edited

Loading

codecov-commenter commented Jul 16, 2020 •

edited

Loading