Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduced benchmarks #2166

Merged
merged 9 commits into from
Jul 13, 2022
Merged

Reduced benchmarks #2166

merged 9 commits into from
Jul 13, 2022

Conversation

stefanosiano
Copy link
Member

@stefanosiano stefanosiano commented Jul 12, 2022

📜 Description

Reduced benchmarks to only run on android/java packages (not on integrations)
On every pull request the benchmarks will be run on one device only.
The full benchmark device suite will be tested nightly.

#skip-changelog

💡 Motivation and Context

The benchmarks currently take ~1 hour to run on saucelabs, more if a device times out. The situation is going to get worse as more tests are added.
To alleviate the situation we run the tests only on one device, so that this workflow should take ~10-15 minutes (plus build time). Also, running benchmarks on just one device reduces the risk of timeouts from saucelabs.

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

We will monitor the situation and will make further changes in the future if needed, but at the moment these changes should be enough

…rations)

added a check on "#skip-benchmarks" in the pr description
…rations)

added a check on "#skip-benchmarks" in the pr description
…rations)

added a check on "#skip-benchmarks" in the pr description
…rations)

added a check on "#skip-benchmarks" in the pr description
…rations)

added a check on "#skip-benchmarks" in the pr description
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

Codecov Report

Merging #2166 (2db66ac) into main (99ade31) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #2166   +/-   ##
=========================================
  Coverage     80.91%   80.91%           
  Complexity     3287     3287           
=========================================
  Files           233      233           
  Lines         12043    12043           
  Branches       1594     1594           
=========================================
  Hits           9745     9745           
  Misses         1715     1715           
  Partials        583      583           

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 99ade31...2db66ac. Read the comment docs.

on pr only one device will be tested
nightly all devices will be tested
@stefanosiano stefanosiano marked this pull request as ready for review July 12, 2022 17:43
@@ -1,9 +1,18 @@
name: "Integration Tests - Benchmarks"
on:
schedule:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking if it's necessary, because they will anyway run on every merge/push to main? If there were no changes on main and a cron job kicks in, it's still useless probably, because there were nothing new on main?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may be right on this
let's remove pushing to main: we may have multiple pushes to main daily, so we could end up with a lot of tests running on saucelabs, risking timeouts and slowdowns. Also, we would still run the single benchmark for every pr, so running only nightly is enough at the moment. We can still change it later

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sounds good 👍 just need some kind of reminder on slack or somewhere to check the benchmarks from the nightly builds

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to do it now, but would be cool to either post on slack or automatically create an issue if there were regressions after the nightly build

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually I guess it's pretty straightforward - if a test run failed, we just use some action to create an issue and that's it :D most important is that there shouldn't be flaky tests to not overwhelm us with issues haha

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it shouldn't be the case, even because when a test times out it succeeds 😬

@romtsn
Copy link
Member

romtsn commented Jul 12, 2022

Just one small comment, otherwise LGTM 🚀 You already got my approval anyway

@stefanosiano stefanosiano merged commit 3ad96de into main Jul 13, 2022
@stefanosiano stefanosiano deleted the tests/reduce-benchmark-runs branch July 13, 2022 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants