-
Notifications
You must be signed in to change notification settings - Fork 42
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
refactor(RHINENG-5926) ZeroState checks for RHEL systems #2070
Conversation
Commits missing Jira IDs: |
0ecec39
to
833cc00
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2070 +/- ##
==========================================
- Coverage 68.10% 67.84% -0.26%
==========================================
Files 131 131
Lines 3414 3418 +4
Branches 1064 1062 -2
==========================================
- Hits 2325 2319 -6
- Misses 1089 1099 +10 ☔ View full report in Codecov by Sentry. |
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 get the zero state even though I have systems, have some suggestions
let isEdgeParityEnabled = true; | ||
let hasEdgeDevices = false; | ||
let hasConventionalSystems = false; | ||
expect(checkForAccountSystems(isEdgeParityEnabled, hasEdgeDevices, hasConventionalSystems)).toBe(false) |
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.
As we have migrated to RTL, would it be better to assert on the UI element itself instead of the checkForAccountSystems function result?
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.
Originally that was the plan, however the routing page is essentially just a fed module now. So we're really just making sure the right state renders when passing in the correct items
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.
LGTM! I did not have accounts to test that have only RHEL systems and do not have any such systems. If you have accounts and feel testing is needed, I am ready to test them out
Updates zero state to consume children, and change the api request to check for RHEL systems as opposed to ALL systems