Bug 1545730 - Enable raptor tests on Fenix #1774
Conversation
ghost
assigned 
JohanLorenzo
changed the title
JohanLorenzo
force-pushed
the
raptor
branch
5 times, most recently
from
3c41557 to
ca94bf5
Compare
|
Raptor jobs tested at https://tools.taskcluster.net/groups/Srw96dYvTzymHitVLKx9GA |
JohanLorenzo
changed the title
|
r? @davehunt |
|
@JohanLorenzo I'm not familiar enough with this code to provide a review, is there someone else that can take it? I would also suggest @ionutgoldan @rwood-moz take a look over these changes, but shouldn't block. |
There was a problem hiding this comment.
there is a bit of rubber stamping going on here- this is easy to follow and read, likewise it is a good start for us to edit in the future.
I have a lot of nits I would like to mention regarding adding comments. I only have one item I did add as a followup todo which seemed most relevant and actionable.
| _DEFAULT_TASK_URL, signing_task_id, DEFAULT_APK_ARTIFACT_LOCATION | ||
| ) | ||
| architecture, _ = get_architecture_and_build_type_from_variant(variant) | ||
| worker_type = 'gecko-t-ap-perf-p2' if force_run_on_64_bit_device or architecture == 'aarch64' else 'gecko-t-ap-perf-g5' |
There was a problem hiding this comment.
we need to fix this (as a followup is ok) for both refbrow and fenix- we only use gecko-t-ap-perf-g5, but for power/battery tests we need gecko-t-ap-batt-g5|p2. It isn't a priority to have speedometer power running, but having the plumbing so we can edit the test name/definition and it just works would be really nice.
There was a problem hiding this comment.
Good catch! I'll handle it in a followup.
app/build.gradle
Outdated
| doLast { | ||
| println "nightly: " + groovy.json.JsonOutput.toJson(GeckoVersions.nightly_version) | ||
| println "beta: " + groovy.json.JsonOutput.toJson(GeckoVersions.beta_version) | ||
| println "release: " + groovy.json.JsonOutput.toJson(GeckoVersions.release_version) |
There was a problem hiding this comment.
Do we need any of these other than nightly? If not, perhaps we should remove the others until they're needed.
One development behaviour I really like is having the simple obsolete-code-heuristic "if this isn't used [anymore], it can be deleted". If we add functionality because we'll need it in the future, then it's easier for obsolete code to stick around because it's not immediately obvious that it's not "code for the future".
When we need to find the beta geckoview version, we'll definitely be doing a new PR anyways, so having this as part of this changeset isn't saving us from a PR.
Not a blocker for this PR, but just a pattern that I appreciate. What do you think?
There was a problem hiding this comment.
I agree with you, it's not used yet. I heard the first Fenix release will be on Geckoview 68 which will go beta soon. That said, we'll need to define new jobs anyway. I removed these extra lines.
| arch in ('arm', 'aarch64') and | ||
| is_master_push | ||
| architecture in ('arm', 'aarch64') and | ||
| SHORT_HEAD_BRANCH == 'master' |
There was a problem hiding this comment.
Why do you need both is_master_push and SHORT_HEAD_BRANCH == 'master'? Aren't they confirming the same underlying fact?
There was a problem hiding this comment.
Good catch. is_master_push is misleading. I just renamed it into is_push
| @@ -436,6 +452,115 @@ def craft_push_task( | |||
| }, | |||
| ) | |||
|
|
|||
| def craft_raptor_tp6m_cold_task(self, for_suite): | |||
|
|
|||
| def craft_function(signing_task_id, mozharness_task_id, variant, gecko_revision, force_run_on_64_bit_device=False): | |||
There was a problem hiding this comment.
We never call _craft_raptor_task directly.
Perhaps instead of defining that function, then making a closure that wraps it with default values, why don't we just move the defaults into _craft_raptor_task itself and remove the need for craft_raptor_tp6m_cold_task?
Perhaps this is similar to my argument about printGeckoviewVersions - it seems like we're adding complexity ahead-of-time before it's warranted
There was a problem hiding this comment.
_craft_raptor_task() is going to be called at several places real soon, when new job types are enabled (like they are in r-b). I'd prefer this function around. This will make job addition easier.
Pull Request checklist
Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)N/ATests: This PR includes thorough tests or an explanation of why it does notN/AChangelog: This PR includes a changelog entry or does not need oneN/AAccessibility: The code in this PR follows accessibility best practices or does not include any user facing featuresN/A