-
Notifications
You must be signed in to change notification settings - Fork 5
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
AzureIntegrationTests share one billing profile #1702
base: develop
Are you sure you want to change the base?
AzureIntegrationTests share one billing profile #1702
Conversation
I first converted AzureIntegrationTest to JUnit5. This was necessary in order to create @BeforeAll and @afterall methods to initialize a single Azure billing profile for use in all tests. Expected benefits: - If one test in the suite fails to delete its billing profile on tear-down, all other tests in the suite currently fail fast when trying to create datasets with errors like "Cannot reuse existing application deployment integrationapp from profile … with a different profile …". This change should allow for more precise failure signaling, and increase the efficacy of retries. - Serve as a model for sharing test initialization within a class, not currently used elsewhere in TDR. Overall integration test runtime can take 2+ hours, and sharing set-up may help drive that down while incentivizing writing more granular tests when appropriate rather than overloading existing ones.
@Category(Integration.class) | ||
public class AzureIntegrationTest extends UsersBase { | ||
@Tag(Integration.TAG) | ||
@TestInstance(TestInstance.Lifecycle.PER_CLASS) |
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.
This annotation is necessary to use @BeforeAll
and @AfterAll
annotations on non-static methods.
AzureIntegrationTest had a TestJobWatcher included as a @rule, which is no longer supported in JUnit 5. Instead, we can extend the test class with new TestJobWatcherExtension, which is compatible with JUnit 5. Also refactored two assertions which still used JUnit 4 methods.
To make debugging easier.
Quality Gate passedIssues Measures |
- name: "Run integration tests via Gradle" | ||
uses: broadinstitute/datarepo-actions/actions/main@0.73.0 | ||
with: | ||
actions_subcommand: 'gradleinttest' | ||
pgport: ${{ job.services.postgres.ports[5432] }} | ||
test_to_run: 'testIntegration' | ||
test_filter: 'AzureIntegrationTest' ## DO NOT MERGE. |
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 won't check this in, but PSA that this is a supported filter by way of this PR: broadinstitute/datarepo-actions#74
To help triangulate source of flaky integration tests. The valuable part of this work is sharing a billing profile among tests, so removing other possible sources of flakiness.
- Lost a closing quote when converting to multiline string - Typo in docstring
Autowire it into AzureIntegrationTest rather than having the test extend it. Fix an instance where we mistakenly called UsersBase.steward() in a test where we should have used the steward user initialized in test setup.
Previously, the deployed app <-> billing profile association is registered the first time a dataset is created off of the profile. I believe this is producing a race condition / flaky integration tests using the same billing profile. - Added new CreateProfileRegisterDeployedApplicationStep in ProfileCreateFlight - Notably, added this after the profile metadata was saved and made available as a job response in the working map - Expanded docstrings, unit testing of AzureApplicationDeploymentService.getOrRegisterApplicationDeployment
Test failures and flakiness seems to happen… - During dataset creation… - Specifically in CreateDatasetGetOrCreateStorageAccountStep when getting an existing SA (a new condition when billing profiles are shared, previously a new one was created with each test case)… - Specifically in the call to azureBlobStorePdao.enableFileLogging
Otherwise, test tear-down for a different test may unsuccessfully try to delete them -- observed for releaseSnapshotId specifically, which is not initialized in all tests.
I first converted AzureIntegrationTest to JUnit5.
This was necessary in order to create @BeforeAll and @afterall methods to initialize a single Azure billing profile for use in all tests.
Expected benefits: