-
Notifications
You must be signed in to change notification settings - Fork 18
[DPE-8315] Split integration and release tests #702
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
Conversation
13484ae to
f4dc563
Compare
|
Apparently, all previous |
paulomach
left a comment
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.
Solid. One thing I wish to change is the spread tasks upgrade test names. Any good reason to use date instead of the revision, other that we have more than one revision per date? I would rather stick with the amd64 revision number as it will be there for all.
No other reason than the one you described: there are multiple revisions per every Naming is completely arbitrary and it can be changed to whatever we want. Do you rather do AMD revision naming? Keep in mind that the AMD revision is not always the first one to be promoted to |
| build: | ||
| name: Build charm | ||
| uses: canonical/data-platform-workflows/.github/workflows/[email protected] | ||
|
|
||
| integration-tests: | ||
| name: Integration tests | ||
| needs: | ||
| - build | ||
| uses: ./.github/workflows/integration_test.yaml | ||
| with: | ||
| artifact-prefix: ${{ needs.build.outputs.artifact-prefix }} | ||
| secrets: inherit | ||
| permissions: | ||
| contents: write # Needed for Allure Report |
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.
since we're running integration_test.yaml on PR and on release, can we keep it in ci.yaml and call ci.yaml in release.yaml to avoid duplication (and eliminate the possibility of debugging differences between the two)?
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 think having a strict separation between workflows that are reusable (i.e. trigger-able upon workflow_call) and workflows that are not will help us identify which ones could be, eventually, promoted to some form of shared workflow setup.
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.
Anyway will do, let's not block over details.
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.
duplicating ci.yaml into 3 separate workflows for the pull_request, push, and schedule events is, imo, an antipattern and one that we've intentionally moved away from in the past
it adds unnecessary duplication & complexity and leads to issues like nightly tests not being consistent with pr tests
I don't understand how that change is necessary/relevant to this pr—the existing architecture can be preserved in a straightforward way
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.
Duplicating ci.yaml into 3 separate workflows for the pull_request, push, and schedule events is, imo, an antipattern and one that we've intentionally moved away from in the past
Workflows are not duplicated, because each of them target a different subset of tests / validations:
- PR workflow: linting + unit tests + alert tests + integration tests.
- Release workflow: integration tests + release tests + release lib + release charm.
- Schedule workflow: integration tests + release tests.
As you can see, each workflow has the specific set of steps that make sense for them, while avoiding duplication by extracting those steps that are shared across workflows (i.e. integration tests and release_test) to their own reusable ones.
If you are un-happy with the code duplication between both reusable workflows: trust me, I am too. I suggested you to move the shared Python code to a couple of Python scripts in order to avoid having everything in line, and you refused arguing maintainability concerns (see comment).
tests/integration/release/high_availability/high_availability_helpers.py
Outdated
Show resolved
Hide resolved
if we're going to use revision naming instead of dates, I would pick the alphabetically first revision to match the pattern used in our github release tags https://github.com/canonical/data-platform-workflows/blob/fa480d077b4e5485efcae80d114ff37414b0a0f3/_cli/data_platform_workflows_cli/craft_tools/promote_legacy.py#L245-L248 but imo date is fine |
ok, we stick with dates |
245eea5 to
e23487b
Compare
|
PR branch rebased from |
astrojuanlu
left a comment
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.
Aside from @carlcsaposs-canonical 's comment on the release tests not being included in the Allure report, this LGTM
e23487b to
4fc8f84
Compare
This PR proposes the split of integration and release tests into different directories. Splitting tests into different folders (even within the same
tests/integrationparent one), allow us to define separated spread suites, in order to execute the suites on different GHA events:pull_requestandscheduleevents.pushevent targeting the main branch.Additional changes
tests/integration/releasefolder, making it test every stable revision to date.workflow_calltrigger. Its only consumer will call integration_tests.yaml instead.