-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
Chore: Removing integration tests #2916
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.
Before we can merge this, I think we should
- Add an entry to the internal decision log
- Add critical files we shouldn't change by mistake to
no-changes-in-high-risk-files.sh
. - The failure message in
no-changes-in-high-risk-files.sh
should point you tomake test-alamofire
andmake test-homekit
.
Maybe doing some of these steps in subsequent PRs is also okay. Up to you.
I'll admit I haven't really looked at their results; what issues do they cause? Is it something we could run only after merging to main instead of for every PR push? |
TBH, I cant figure it out the problem, this tests went from working flawlessly to failing every time in CI with no apparent reason. |
@armcknight, we have had these integration tests for roughly a year, and they didn't surface any bugs in our code. We have them to ensure we are not breaking the functionality of UIViewControllers, NSURLSession, NSData, after swizzling. Right now, we know that the swizzling is working fine, and we didn't have to touch the swizzling logic for a year or so. Instead of maintaining these integration tests, we decided to lock these files with no-changes-in-high-risk-files. The next time we have to do some changes, we can add the integration tests again or run them manually. |
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 for doing this @brustolin 👏
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
…coa into chore/test-alamofire
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 @brustolin 👍. LGTM
We don't run the test-alamofire.sh and test-homekit.sh anymore in CI, see GH-2916 and decision log . Both scripts don't even work anymore. We can remove them.
Both Alamofire and Home Assistant integration tests are causing more trouble then helping, we will remove this CI tests.
#skip-changelog