-
Notifications
You must be signed in to change notification settings - Fork 71
✨ Add regression test cases for DeploymentConfig options #2493
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?
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2493 +/- ##
==========================================
- Coverage 73.68% 68.70% -4.99%
==========================================
Files 102 102
Lines 8471 8601 +130
==========================================
- Hits 6242 5909 -333
- Misses 1749 2220 +471
+ Partials 480 472 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
anik120
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.
🎉 thanks for the PR!
I wonder if we loose anything if we just combine all of the options into one?
This is a lot of test manifests we'll have to store in the repository if we break them down individually.
From POV it's sufficient to include one test with ALL of the configuration options, and list in a comment what the individual options are.
|
Yeah that's totally fair. I just was following the objectives as laid out in the OPRUN ticket but I had wondered the same thing, that just the combined case is really needed. |
Updates generate-manifests.go to include a test case for all option types in DeploymentConfig. Updates the testdata/expected-manifests/ directory with all the new test case manifests. Signed-off-by: Tayler Geiger <tayler@redhat.com>
Description
Updates
generate-manifests.goto include test cases for each option type in DeploymentConfig as well as a test case with all DeploymentConfig options.Updates the
testdata/expected-manifests/directory with all the new test case manifests.Large number of file changes is entirely due to all the new manifests for each test case in
testdata/expected-manifests/. Review should mainly focus ongenerate-manifests.gochanges.Reviewer Checklist