-
Notifications
You must be signed in to change notification settings - Fork 124
[WIP] test: add e2e test #1359
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
base: main
Are you sure you want to change the base?
[WIP] test: add e2e test #1359
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.
Code Review
This pull request introduces a new end-to-end (e2e) test job to the Packit CI configuration and updates existing test targets to newer Fedora versions. The addition of e2e tests is a valuable step towards improving test coverage. The changes are generally clear, but there are a few points that need clarification or modification, particularly regarding the source of the test definitions and the test image.
Summary of Findings
- External Test Repository URL: The
fmf_url
for the new e2e tests points to a personal GitLab repository. This should ideally point to an official project repository for long-term stability and maintenance. (Commented) - Test Image Source Clarification: The
TIER1_IMAGE_URL
islocalhost/bootc:e2e
. The mechanism for how this image is built and made available to the test job needs clarification. (Commented) - Scope of Architectures for E2E Tests: The new e2e tests target x86_64 and aarch64. It would be good to understand if other architectures supported in builds (like ppc64le, s390x) are planned for e2e testing. (Commented)
- PR Title [WIP]: The pull request title includes
[WIP]
. This should be removed when the PR is ready for final review and merging. (Not commented due to review settings: severity low) - Identifier Rename: The
identifier
for an existing job was changed fromintegration-test
tointegration
. This is a minor, acceptable change. (Not commented due to review settings: severity low) - Fedora Target Updates: The update of test targets from Fedora 41 to Fedora 42, including relevant architectures, appears to be a standard and positive version progression. (Not commented due to review settings: severity low)
Merge Readiness
This pull request is currently marked as a Work In Progress ([WIP]), and the feedback provided highlights a few areas that require attention before it can be considered ready for merging. Specifically, the source of the FMF test definitions (fmf_url
) should be an official repository, and clarification is needed on how the TIER1_IMAGE_URL
is provisioned. Addressing these points, along with considering the scope of architectures for e2e tests, will strengthen the contribution. As a reviewer, I am not authorized to approve pull requests, but I recommend addressing the high
and medium
severity issues before this PR moves forward. Once these are resolved and the [WIP]
status is removed, it should be in a much better state for merging.
.packit.yaml
Outdated
|
||
- job: tests | ||
trigger: pull_request | ||
fmf_url: https://gitlab.com/xiaofwan/bootc-workflow-test.git |
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.
The fmf_url
for the new e2e
test job currently points to a personal GitLab repository (https://gitlab.com/xiaofwan/bootc-workflow-test.git
).
Is this intended as a temporary placeholder during development? For long-term maintainability and to ensure the tests are versioned and managed alongside the main project's resources, it's generally recommended that such URLs point to an official, project-owned repository. Could this be updated before the PR is finalized?
.packit.yaml
Outdated
tf_extra_params: | ||
environments: | ||
- variables: | ||
TIER1_IMAGE_URL: localhost/bootc:e2e |
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.
The TIER1_IMAGE_URL
for the e2e tests is set to localhost/bootc:e2e
. This implies the image is expected to be available locally within the CI runner's environment.
Could you clarify the workflow for this image?
- Is it built by another job defined within this
.packit.yaml
as part of the PR checks? - Or is it expected to be pre-built and present in the CI environment through some other mechanism?
Understanding its origin would be helpful for maintainability and troubleshooting.
.packit.yaml
Outdated
targets: | ||
- centos-stream-9-x86_64 | ||
- centos-stream-9-aarch64 | ||
- centos-stream-10-x86_64 | ||
- centos-stream-10-aarch64 | ||
- fedora-41-x86_64 | ||
- fedora-41-aarch64 | ||
- fedora-42-x86_64 | ||
- fedora-42-aarch64 | ||
- fedora-rawhide-x86_64 | ||
- fedora-rawhide-aarch64 |
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.
The new e2e
test job (lines 88-110) specifies targets for x86_64
and aarch64
across various CentOS Stream and Fedora versions. The copr_build
job in this file (as per lines 45-50) also includes ppc64le
and s390x
for Fedora builds.
Are there plans to expand the e2e test coverage to these other architectures (ppc64le
, s390x
) in the future, or is the current scope for e2e tests intentionally focused on x86_64
and aarch64
for now? This clarification would be useful for understanding the e2e testing strategy.
Is testing farm still using ec2 Speaking of GHA, also as part of the CNCF migration we now have access to larger runners and I think it would make sense to use those for at least some workload. In general (as this relates to #1358 too) I believe for the forseeable future we will have multiple CI systems and while it's complicated I think it makes sense to try to ensure that we've factored out our testing such that it can be somewhat infrastructure agnostic. At least for baseline testing flows, which a lot of this is right? See https://cloud-native.slack.com/archives/C014YQ8CDCG/p1749065398627899 for a chat around the CNCF GHA runners. |
Will try |
To me one of the biggest problems here is that To flesh this out a bit more what I think would work well is:
Here basically that SDK/test container (which would have e.g. |
Build RPM with moke and re-build bootc image with new bootc RPM Signed-off-by: Xiaofeng Wang <[email protected]>
Add two anaconda and two bib tests