Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Bug 1788643 - Add a nightly cron task for performance tests. #26761

Merged
merged 4 commits into from
Sep 1, 2022

Conversation

gmierz
Copy link
Contributor

@gmierz gmierz commented Sep 1, 2022

This patch adds a separate nightly cron task for the performance tests so we only run them once a day instead of twice.

@gmierz gmierz requested review from a team as code owners September 1, 2022 12:08
@gmierz gmierz requested review from escapewindow and removed request for a team September 1, 2022 12:08
@gmierz
Copy link
Contributor Author

gmierz commented Sep 1, 2022

@escapewindow hello! Are you able to trigger the new nightly-test cron from this PR? I tried running taskcluster locally but I keep running into gradle failures.

@escapewindow
Copy link
Contributor

@escapewindow hello! Are you able to trigger the new nightly-test cron from this PR? I tried running taskcluster locally but I keep running into gradle failures.

I am not.
Some approaches for testing, assuming we're ok with this landing without fully testing:

taskgraph

  1. craft a nightly-test params file by copying and editing https://github.com/mozilla-mobile/fenix/blob/main/taskcluster/test/params/main-repo-cron-nightly.yml to s,nightly,nightly-test
  2. taskgraph target-graph -p <path to params file> should then give us the list of tasks that would be scheduled. This won't actually run anything, but we'll see what's scheduled. We can use the -J option if we want the full task definitions in json form.
  3. continue with review if that works, otherwise fix until it gives the proper set of tasks.

manual cron

  1. land as-is, after review
  2. we add this to the set of manually triggerable cron targets
  3. we can then manually trigger it to make sure it runs properly
  4. celebrate, or fix/backout

or if we want to avoid having this go live, in case the test fails, we can change (1) to "set the when to an empty list [] before landing", so this only triggers via manual hook. Once it works properly, we can add the when back in.

hope

  1. land as-is, after review
  2. wait for cron to trigger, hope it works
  3. celebrate, or fix/backout

@gmierz
Copy link
Contributor Author

gmierz commented Sep 1, 2022

Thank you for the detailed explanations @escapewindow, I'll start by trying the first one and go from there.

@gmierz
Copy link
Contributor Author

gmierz commented Sep 1, 2022

@escapewindow I'm also hitting a Gradle/Kotlin build failure with the target-graph command so I can't test this locally and we'll have to land as-is with option (2). I've pushed a change to the when setting.

@MozillaNoah
Copy link
Contributor

Before this lands, please squash all of the commits into one.

@gmierz
Copy link
Contributor Author

gmierz commented Sep 1, 2022

Before this lands, please squash all of the commits into one.

@MozillaNoah do you have any docs for that? Also, is there a way to have the landing bot handle that?

@MozillaNoah
Copy link
Contributor

@MozillaNoah do you have any docs for that? Also, is there a way to have the landing bot handle that?

I can't immediately find a doc for specifically squashing, but here is our doc for making healthy commits. We generally also open an issue before making a pull request and include that in the commit message and pull request title for ease of tracking.

There is actually a way for the bot to squash your commits! Instead of using the needs:landing label, you can also utilize needs:landing-squashed. The merge bot will collapse all of your commits for you into a single commit, and all of the messages will be retained.

That being said, the main purpose here is just to keep our commit history as clean and succinct as possible. When looking at our git history, we want to find a single commit to attribute a codebase change to. Multiple commits for a single task/fix/bug will make it hard to discern which commit does what and increases the Git history stack. Similarly, it makes more sense to manually squash commits because otherwise the message for the final squashed commit could be something like:

Fix [insert issue here]
Address PR feedback
Address 2nd round of feedback
Fix breaking test
Remove unnecessary new line

When it could instead just be a single commit with something like Closes #123456 - Fix crash on homepage caused by top sites.

(I hope that all makes sense)

@gmierz
Copy link
Contributor Author

gmierz commented Sep 1, 2022

Thanks @MozillaNoah, that's very useful info! I'll make use of the needs:landing-squashed label here.

@escapewindow
Copy link
Contributor

@escapewindow I'm also hitting a Gradle/Kotlin build failure with the target-graph command so I can't test this locally and we'll have to land as-is with option (2). I've pushed a change to the when setting.

https://moz-releng-docs.readthedocs.io/en/latest/procedures/mobile_automation_setup.html#how-to-set-up-taskgraph-for-mobile may help for the future.

@gmierz
Copy link
Contributor Author

gmierz commented Sep 1, 2022

@escapewindow I'm also hitting a Gradle/Kotlin build failure with the target-graph command so I can't test this locally and we'll have to land as-is with option (2). I've pushed a change to the when setting.

https://moz-releng-docs.readthedocs.io/en/latest/procedures/mobile_automation_setup.html#how-to-set-up-taskgraph-for-mobile may help for the future.

Yes, thanks! I was using the cron/decision task CI setup steps and it was not working.

@gmierz gmierz added the pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify] label Sep 1, 2022
@mergify mergify bot merged commit 3033b26 into mozilla-mobile:main Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:needs-landing-squashed PRs that are ready to land (squashed) [Will be merged by Mergify]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants