-
Notifications
You must be signed in to change notification settings - Fork 111
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
Run basic system test on GitHub Actions #1731
base: master
Are you sure you want to change the base?
Run basic system test on GitHub Actions #1731
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.
Thanks !
I actually agree to disagree a bit here.
To be honest, I think this change will add more friction than it will help (as it is).
Looking at checks on this PR, I can see that the test
(actually basic system tests only) job takes ~double the amount of time it takes to run the build
job (which also runs all unit tests, so naming here might need a refresher).
I agree that they run in //, but still, 15 minutes for the Windows one, it adds ~6 minutes on top of current ~9 minutes (for Windows build one). They are not required, but still, it never feels nice merging a PR with incomplete checks, and also because of the way we rebase / force push, we sometimes end up in situations where we have to wait on all checks multiple times in a row.
I'd say it could worth it if this was really catching much more errors on PRs compared to what UTs are catching right now, but the limited commands used in the basic tests, are almost never failing to run through completion from PR to PR, so I don't think it will catch a lot more. What mostly breaks in STs is due to desynchronization with the fixtures (some automation improvement there might help at some point to mitigate this) and only way to notice that is to run the full suite.
What I feel could be done instead is to run a ST job to run all system tests on every push to master (i.e not cron triggered once a day as it is now, but on every push to master). This way we would catch ST failures / bugs early on, more than by just running an ultra small subset of commands, without impacting PR swiftness.
Thoughts welcome :)
Great ideas! Yes definitely agree. And actually I wanted to update the PR description, but then got busy with other things.. You're absolutely right, the basic test (as is) on Windows and macOS takes way too long to be run on every PR. Maybe the Linux one could be optimized to run in about the same amount of time the other (existing) checks take. That way we would not slow down the PR/check cycle. I'll think a bit more about this and update later. Marked as do-not-merge for now. Thanks! |
This adds a "basic" system test (creating a miniapp, and generating both Android and iOS containers). While this test is of course not as comprehensive as the full suite, it will help us catch a large number of potential bugs much earlier. It has already happened several times in the past that we were surprised by a 9pm system test failure.
Also fixed a few issues with command line args running tests on Windows.
To run the test:
It takes around 3 minutes on my local machine. Since it will run in parallel with other jobs on CI, it should be fast enough to run with every PR. We can test this out for a week or two and see how it goes.
This is one more step towards fully migrating to GitHub Actions CI, and if this proves to be reliable, we can (finally) remove the Azure tests in the near future (after running the full system test suite on schedule).