Fix supports_helper stub_supports to handle unsupported_reason #22976
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@miq-bot cross-repo-test /all |
miq-bot
pushed a commit
to ManageIQ/manageiq-cross_repo-tests
that referenced
this pull request
Apr 5, 2024
From Pull Request: ManageIQ/manageiq#22976
17 tasks
kbrock
force-pushed
the
supports_unsupports_before
branch
from
b22c2af to
517f3a6
Compare
kbrock
changed the title
kbrock
force-pushed
the
supports_unsupports_before
branch
from
517f3a6 to
bc0151b
Compare
|
update:
WIP:
|
agrare
reviewed
Apr 5, 2024
| allow_any_instance_of(model).to receive(:supports?).and_call_original | ||
| allow(model.supports_features).to receive(:[]).and_call_original |
Fryguy
reviewed
Apr 5, 2024
Fryguy
reviewed
Apr 5, 2024
We are now getting away from supports?(:supported_feature). Instead we are calling unsupported_reason(:supported_feature) It cuts down on our calls into the supports_feature_mixin. Before ====== When using stub_supports, unsupported_reason(:supported_feature) was not stubbed (and unpredictable) supports? was stubbed After ===== We are stubbing at the feature definition layer all code in unsupported_reason is run/tested all code in supports? is run/tested This puts more code under test and allows flexibility for refactoring Notes ===== Wasn't thrilled about kwarg supports, but a few tests still use that. I've changed them all but it wasn't too much work to keep this working so we can backport with minimal fuss This code better tests our features that defer to another feature
kbrock
force-pushed
the
supports_unsupports_before
branch
from
bc0151b to
235eed6
Compare
|
Checked commit kbrock@235eed6 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint |
|
un-WIP: cross repo is failing for ui-classic (and ui-services). It looks like it some configuration issue. Need a second opinion on the ui-classic failure, but I'm happy with this code at this time. |
kbrock
changed the title
|
@miq-bot cross-repo-test manageiq-ui-classic trying just to run ui-classic |
miq-bot
pushed a commit
to ManageIQ/manageiq-cross_repo-tests
that referenced
this pull request
Apr 5, 2024
From Pull Request: ManageIQ/manageiq#22976
|
@Fryguy Can we merge?
|
agrare
approved these changes
Apr 24, 2024
|
This PR has been effectively backported to |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
requires:
required by:
Overview
We are now getting away from supports?(:supported_feature). Instead we are calling unsupported_reason(:supported_feature)
It cuts down on our calls into the supports_feature_mixin.
Before
When using
stub_supports,unsupported_reason(:supported_feature)was not stubbed (and unpredictable)supports?(:all_features)was stubbedAfter
We are stubbing at the feature definition layer
all code in
unsupported_reasonis run/tested (not stubbed)all code in
supports?is run/tested (not stubbed)This puts more code under test and allows flexibility for refactoring
Notes
Wasn't thrilled about
stub_supports kwarg supports:I've removed the tests that still use that, but keeping that parameter as-is so
this is more backport friendly.
This code better tests our features that defer to another feature
this is a little more finicky since it stubs supports for only the class specified. good: you can stub different classes with different values. bad: have to stub leaf classes