feat(mr): create mr modal #2828
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2828 +/- ##
==========================================
- Coverage 77.40% 77.40% -0.01%
==========================================
Files 1101 1102 +1
Lines 23176 23264 +88
Branches 5843 5870 +27
==========================================
+ Hits 17940 18008 +68
- Misses 5236 5256 +20
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
gitdallas
force-pushed
the
feat/mr-creation
branch
from
5991ae8
to
5685557
Compare
Signed-off-by: gitdallas <[email protected]>
gitdallas
force-pushed
the
feat/mr-creation
branch
from
5685557
to
f28a71a
Compare
|
Approving, I'll follow up on some minor things we discussed as part of #2829 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mturley The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| cy.findByText('Create model registry').click(); | ||
| // Fill in the form with invalid data | ||
| cy.get('#mr-name').clear(); | ||
| cy.get('#mr-host').clear(); | ||
| cy.get('#mr-port').clear(); | ||
| cy.get('#mr-username').clear(); | ||
| cy.get('#mr-password').clear(); | ||
| cy.get('#mr-database').clear(); | ||
| cy.get('#mr-database').blur(); | ||
| // Assert the submit button is disabled | ||
| cy.findByTestId('modal-submit-button').should('be.disabled'); | ||
| // Assert that error messages are displayed | ||
| cy.findByTestId('mr-name-error') | ||
| .should('be.visible') | ||
| .contains( | ||
| "Must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character", | ||
| ); | ||
| cy.findByTestId('mr-host-error').should('be.visible').contains('Host cannot be empty'); | ||
| cy.findByTestId('mr-port-error').should('be.visible').contains('Port cannot be empty'); | ||
| cy.findByTestId('mr-username-error').should('be.visible').contains('Username cannot be empty'); | ||
| cy.findByTestId('mr-password-error').should('be.visible').contains('Password cannot be empty'); | ||
| cy.findByTestId('mr-database-error').scrollIntoView(); | ||
| cy.findByTestId('mr-database-error').should('be.visible').contains('Database cannot be empty'); |
There was a problem hiding this comment.
@dpanshug Selectors should be part of page objects so they are reusable when we get to e2e tests.
There was a problem hiding this comment.
@christianvogt FYI I addressed this in updates to #2829
| .contains( | ||
| "Must consist of lower case alphanumeric characters or '-', and must start and end with an alphanumeric character", | ||
| ); |
There was a problem hiding this comment.
Asserting static text as defined in in our react components is often a bad idea because it's a 1:1 relationship and will only fail if we change the react component.
There was a problem hiding this comment.
Ack. I plan to give these tests a second pass as part of adding more in #2829, I'll address this feedback there and request a review from you for that one when it's ready if you don't mind.
There was a problem hiding this comment.
@christianvogt looking back at this, I think it has value beyond what you're saying -- we shouldn't just assert that components render certain text for the sake of it, but this is trying to assert that the error is shown vs not shown. We could do it with a testid instead if you prefer but imo it doesn't matter much as long as the test is asserting logic and not just content. This test will fail if the form validation logic breaks.
There was a problem hiding this comment.
My concern was the contains check and not so much the check that the error node was visible since this is the only available error text that's shown in this node.
Note that if the error text was dynamic from a server response, I would agree with asserting the text presence.
There was a problem hiding this comment.
@christianvogt FYI I addressed this in updates to #2829
Closes: RHOAIENG-2230
Description
Create MR form/modal
How Has This Been Tested?
Created an MR, checked form cannot be submitted if form validations exist.
Test Impact
Request review criteria:
Modal opens, form looks right, validation shows up. form can be submitted with good content.
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main