-
Notifications
You must be signed in to change notification settings - Fork 4
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
Detect CI environment and send associated data along with the test suite #21
Conversation
- Updated setup to use Gradle configuration to pass environment variables directly to instrumentation arguments instead of using BuildConfig. - Simplified the instrumentation test collector as InstrumentedTestCollector is no longer abstract and now handles environment variables internally, thus removing the need to create a custom test collector class.
Added TestDataUploader and TestObserver interfaces to allow custom implementations and also improve flexibility and testability.
Extracted TestListener implementation to a new class UnitTestListener from UnitTestCollectorPlugin for better separation of concerns.
Extracted RunListener implementation to a new class InstrumentedTestListener from InstrumentedTestCollector for better separation of concerns and testability.
The commit adds support for uploading environment variables from various CI system - Buildkite, CircleCI and GitHubActions via accessing variables each system provides. If no CI system is detected, it sends default environment data.
Moved validation to check for the presence of the 'BUILDKITE_ANALYTICS_TOKEN' environment variable before creating instances of `UnitTestListener` and `InstrumentedTestListener`. This change ensures that no test data is collected or uploaded without a valid API token unnecessarily.
2259a45
to
40f9300
Compare
40f9300
to
5531f25
Compare
5531f25
to
ec0838d
Compare
57170be
to
ea62982
Compare
ea62982
to
b581775
Compare
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 don't know kotlin, and I have no thoughts on the refactoring, but the added CI environment support for GitHub looks good and I checked the ENV vars against the values in the GitHub docs 👍
Seeing as this PR is quite a significant refactor, is it possible to integration test these changes before they are released? Again, I don't know how any of this works, but is it possible to build a collector with these changes and verify that everything works as intended and the test results are received by Buildkite on a local test run with an API key from a nominated production Suite?
Hi @niceking, Thank you for your patience regarding the delayed response. We have an example project that uses the test collectors which should be used to test any changes locally before pushing up a PR. This project provides a working representation of the SDK. Detailed steps on how to test changes locally are noted in the Contribution guide. Additionally, we have a CI workflow that tests changes via CI runs. This workflow sends the example project's instrumented and unit test data to the analytics suite using the token provided in secrets. @KatieWright26 might be aware of the associated dev account with the token. It's important to note that this workflow is/should be disabled (and should only be enabled and when there is a need to test the changes) as it is set up to fail due to intentionally failing tests in the example project. I have added some comprehensive unit tests to verify environment variables access for different CI systems. Although, there are some additional tests that can now be written due to this refactoring (as it sets up more testable architecture) that I couldn't cover due to the limited time available before I transitioned to another engagement. I will open an issue to ensure these tests are added and tracked for future completion. Once this PR is merged, it will publish the changes to the SNAPSHOT version on Maven. This means that the changes will be available for testing in other projects that use the SDK, not just the example project before we actually release the changes. Following that and the setup instructions in the README, we can easily test the new features without needing to go through the Contribution guide. I hope this answers your questions, but please feel free to ask if you need additional information. |
Thanks for your due diligence around this @thebhumilsoni :) |
This PR adds support for passing environment variables from various CI systems to enrich test reports. It also introduces significant improvements to the collector architecture, enhancing flexibility, testability, and separation of concerns.
💬 Summary of Changes
InstrumentedTestCollector
. New setup uses Gradle configuration to pass environment variables directly through instrumentation arguments instead of using BuildConfig. This simplifies the instrumentation test collector setup as now environment variables are passed toInstrumentedTestCollector
without a need to create a custom test collector implementation.TestDataUploader
andTestObserver
interfaces to allow for custom implementations.TestListener
implementation into a new classUnitTestListener
, improving the separation of concerns and making the UnitTestCollectorPlugin cleaner.RunListener
implementation into a new classInstrumentedTestListener
from InstrumentedTestCollector.TestEnvironmentProvider
andBaseTestEnvironmentProvider
to handle environment variables effectively from various sources such as system properties or Android Bundle arguments.README
and created a newCI_CONFIGURATION
documentation file with detailed setup instructions and examples for configuring environment variables for supported CI platforms.These changes significantly improve the framework's architecture and maintainability.
🧾 Checklist
📝 Items of Note