Skip to content

Conversation

@InsaneZein
Copy link
Collaborator

part of RHCLOUD-31955

@InsaneZein InsaneZein requested a review from fhlavac May 1, 2024 21:09
@patternfly-build
Copy link

patternfly-build commented May 1, 2024

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a typo in the file name. Also, the test seem to be incomplete

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oops I forgot about that one. Looking at the failed test screenshot, it looks like the ErrorBoundary doesn't get rendered. Not sure what's going on there.

import InvalidObject from '../../packages/module/dist/dynamic/InvalidObject';

describe('InvalidObject', () => {
/* eslint-disable no-console */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this one needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh 😅 the problem of copy-paste. I took these out.

import ShortcutGrid from '../../packages/module/dist/dynamic/ShortcutGrid';

describe('ShortcutGrid', () => {
/* eslint-disable no-console */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* eslint-disable no-console */

import SkeletonTable from '../../packages/module/dist/dynamic/SkeletonTable';

describe('SkeletonTable', () => {
/* eslint-disable no-console */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* eslint-disable no-console */


describe('Ansible', () => {
it('renders supported Ansible', () => {
/* eslint-disable no-console */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* eslint-disable no-console */

cy.get('i').should('have.class', 'ansibleSupported-0-2-2');
});
it('renders unsupported Ansible', () => {
/* eslint-disable no-console */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* eslint-disable no-console */

/* eslint-disable no-console */
it('renders the Close button', () => {
cy.mount(<ErrorState errorTitle='Sample error title' errorDescription='Sample error description' />);
cy.get('h4').should('have.text', 'Sample error title');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please check also the description?

it('renders InvalidObject', () => {
cy.mount(<InvalidObject />)
cy.get('[class="pf-v5-c-empty-state"]').should('exist')
cy.get('h1').should('have.text', 'We lost that page');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also check the description and button?

describe('LogSnippet', () => {
it('renders LogSnippet', () => {
cy.mount(<LogSnippet logSnippet='test test code' message='A test message'/>)
cy.get('div div p').should('have.text', 'A test message');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code content could be tested as well

serviceName="Test bundle"
prevPageButtonText="Go to previous page"
/>);
cy.get('div div div h5').should('have.text', 'You do not have access to Test bundle');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please also add the description check?

it('renders UnavailableContent', () => {
cy.mount(<UnavailableContent />)
cy.get('[class="pf-v5-c-empty-state__content"').should('exist');
cy.get('div div div h5').should('have.text', 'This page is temporarily unavailable');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please check the description?

@fhlavac
Copy link
Contributor

fhlavac commented May 2, 2024

@InsaneZein it looks good, just posted some smaller comments. It looks good as a base. It would probably require follow-up tickets to cover all variants of multi-content card and warning modal callbacks. What do you think?

@InsaneZein
Copy link
Collaborator Author

I agree @fhlavac I tried going through the whole document, but had to miss a couple because of lack of bandwidth.

describe('ErrorBoundary', () => {
it('renders the ErrorBoundary ', () => {
cy.mount(<ErrorBoundary headerTitle="My app header" errorTitle="Something wrong happened"/>)
// cy.get('div h1').should('have.text', 'Something wrong happened');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the assertion commented out? Without it we probably even don't need to run the test

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason ErrorBoundary is not getting rendered. I'm not sure what's going on. Could we make fixing this as part of the next ticket?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@InsaneZein sounds good, can you please add a description of the issue to that ticket? Thank you

@fhlavac
Copy link
Contributor

fhlavac commented May 6, 2024

I agree @fhlavac I tried going through the whole document, but had to miss a couple because of lack of bandwidth.

No problem, I've opened a follow-up ticket RHCLOUD-32488 to cover the rest from the document

@@ -0,0 +1,9 @@
import React from 'react';
import ErrorBoundary from '../../packages/module/dist/esm/ErrorBoundary';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import ErrorBoundary from '../../packages/module/dist/esm/ErrorBoundary';
import ErrorBoundary from '../../packages/module/dist/dynamic/ErrorBoundary';

@fhlavac fhlavac merged commit edbca71 into main May 7, 2024
@fhlavac fhlavac deleted the add-basic-cypress-tests branch May 7, 2024 07:56
@github-actions
Copy link

🎉 This PR is included in version 5.2.0-prerelease.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants