-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add more options and optimization for our tests #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once this is merged to main, I will tag a version so we can use that in our repos. Since this is our fork, we can control the tags (correct me if I'm wrong). @goncalossilva and @Doist/android, would appreciate your thoughts on this part as well.
Yes, we can use tags. 👍
src/emulator-manager.ts
Outdated
@@ -27,6 +27,9 @@ export async function launchEmulator( | |||
`sh -c \\"echo no | avdmanager create avd --force -n "${avdName}" --abi '${target}/${arch}' --package 'system-images;android-${apiLevel};${target};${arch}' ${profileOption} ${sdcardPathOrSizeOption}"` | |||
); | |||
|
|||
// set number of cores to 2 (instead of 1 because there are 3 cores on the Github-hoster macOS VM) | |||
await exec.exec(`sh -c \\"printf 'hw.cpu.ncore=2\n' >> ~/.android/avd/"${avdName}".avd"/config.ini`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be configurable, too, with some kind of default (i.e., do nothing)? I feel this is the only change that doesn't generalize as is to submit upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a discussion with the maintainer about this change once I figured this was the difference between our Azure pipeline and the Emulator on GitHub. I will generalize it though, to make it easier to modify this number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest exactly this. 👏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! Added a cores
option now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great investigation @AfzalivE. 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only left one comment, related with what Gonçalo already mentioned above! 👏
Other than that, looks great! 🚀
lib/emulator-manager.js
Outdated
return __awaiter(this, void 0, void 0, function* () { | ||
// create a new AVD | ||
const profileOption = profile.trim() !== '' ? `--device '${profile}'` : ''; | ||
const sdcardPathOrSizeOption = sdcardPathOrSize.trim() !== '' ? `--sdcard '${sdcardPathOrSize}'` : ''; | ||
console.log(`Creating AVD.`); | ||
yield exec.exec(`sh -c \\"echo no | avdmanager create avd --force -n "${avdName}" --abi '${target}/${arch}' --package 'system-images;android-${apiLevel};${target};${arch}' ${profileOption} ${sdcardPathOrSizeOption}"`); | ||
// set number of cores to 2 (instead of 1 because there are 3 cores on the Github-hoster macOS VM) | ||
yield exec.exec(`sh -c \\"printf 'hw.cpu.ncore=2\n' >> ~/.android/avd/"${avdName}".avd"/config.ini`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I saw this from the PR description _ The lib folder is generated by npm._ after posting the comment so it's probably not applicable. 😅
b0db911
to
e980520
Compare
It's in private beta and we don't have access to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @AfzalivE ! Great that everything's parameterizable now 🚀 LGTM!
Thank you both for the review!! |
A question about the OS we're using: macOS is 10x more expensive to run than Linux. Rasto did comment that macOS images were faster, but is it significantly different? |
Unfortunately, the Linux and Windows VMs that GitHub provides don't have hardware virtualization/acceleration support and they're not very reliable above API level 16. Last time I tried Ubuntu with API 29, it didn't boot in 2 hours. Trying again now considering all the other changes I've made since. Edit: Boot completed but then it got stuck. Here's some more background into virtualization support for Linux and Windows: However, since macOS is 10x more expensive. I ran the current Azure pipeline stats to see how much it would cost us, here's the spreadsheet. Spoiler: It's not cheap when the jobs start timing out a lot but this past week, average day had less than 3 hours of jobs. At 4 hours per day, 30 days would be 120 minutes. On a Mac VM: 1200 minutes just for Android runs. Plus if we use matrix to run tests faster (half the time compared to Azure): it's ~ 1.5 billable hours per build. It is possible that this change on GitHub's roadmap for Q1 2021 will bring improvements to the Linux/Windows machines that would allow reliably running the emulator on those OS. |
I didn't look at this in detail earlier but Firebase Test Lab might actually be much cheaper for us. GitHub's macOS VM comes out to $4.8/hr ($0.08/min) and Firebase Test Lab is $1/hour ($0.0166/min) on a virtual device. Plus, they offer 60 minutes per day of virtual device for free. Sharding tests on Firebase would also end up faster and might not incur much more cost than not sharding since compilation would happen on GitHub Actions (ubuntu) so only tests will run on Firebase. Although, I'm not sure how the performance is. I see on Twist that Firebase Test Lab was considered multiple times before. Do you know why we didn't continue pursuing it? |
Great analysis, @AfzalivE! It's a shame that GitHub is approaching this so strangely.... My take is that unless we are talking about different magnitudes of cost, the most valuable aspect is developer time. The more we can shard the tests, the better. The faster they run, the better. So if macOS on GitHub is 2x faster, that is the better choice (even if more expensive). Plus, we can reuse our know-how on GitHub Actions across the rest of Doist. But if Firebase Test Lab is even faster (and allows us to shard more), then that's the better option. Let's use it. 😄 Personally, I would love to see feedback loops brought down to 15 minutes at most. |
In that case we're good! This is true and so far with 4 shards, 2x faster. 6 shards wasn't much of a difference. I didn't go so far as 8 shards though 😅 .
Not sure about this. I didn't try with a physical device on Firebase but I did execute 1 run and it failed in 15 minutes with this error: I think it would be valuable explore this further in the future to speed this up even more. Especially if this would allow us to cache the UI tests as well in the gradle cache in the future. That's around 5 minutes of compile time. There's also a project called Flank that Airbnb uses that would help in sharding on Firebase Test Lab. |
Note: The lib folder is generated by npm. A release branch called
release/v2-doist
has been created from this branch to use these changes until a tag is ready.Once this is merged to main, I will tag a version so we can use that in our repos. Since this is our fork, we can control the tags (correct me if I'm wrong). @goncalossilva and @Doist/android, would appreciate your thoughts on this part as well.
Given the amount of new options here, I wouldn't be surprised if some of these changes get rejected upstream.