Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Jul 21, 2021

WHY are these changes introduced?

I am updating existing tests for Truncate and UnstyledButton components to use the new modern framework.

WHAT is this pull request doing?

Part of test modernization (https://docs.google.com/spreadsheets/d/1GBuEZbOpVYJLNISK7gL69DU8rCxocn5L5GYaKtPXPbU/edit#gid=1498187033), updating tests using {mountWithAppProvider} from 'test-utilities/legacy' to {mountWithApp} from 'test-utilities'.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 21, 2021

size-limit report

Path Size
cjs 142.54 KB (0%)
esm 96.31 KB (0%)
esnext 139.51 KB (0%)
css 33.75 KB (0%)

Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

Two little thinks noted inline. Other than that all looks good to me!

const button = mountWithAppProvider(<UnstyledButton external />);
expect(button.find('button').prop('external')).toBeUndefined();
const button = mountWithApp(<UnstyledButton external />);
expect(button).toContainReactComponent('button');
Copy link
Member

Choose a reason for hiding this comment

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

This should test for the absence of external

Copy link
Author

@ghost ghost Aug 11, 2021

Choose a reason for hiding this comment

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

Hi! Yes, I wasn't sure about this one, for that reason I opened but I didn't add anyone yet until I solve it now that I came back from vacation. My doubt is because external is only used in the UnstyledLink that appears when external is passed. If it is not passed, it renders a button (For that reason, I was asserting by checking the button. I was thinking to add an not.renders around the UnstyledLink with undefined external). If I insert this as a prop, VSCode says that external doesn't exist in button and for that reason I cannot do the assertion.
Screen Shot 2021-08-11 at 12 57 55 PM. Same with the one below. Do you have any suggestion around this?

Copy link
Author

Choose a reason for hiding this comment

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

I updated the PR with the idea i had in mind, let me know what do you think @BPScott

<UnstyledButton url={mockUrl} external disabled />,
);
expect(button.find('a').prop('external')).toBeUndefined();
expect(button).toContainReactComponent('a');
Copy link
Member

Choose a reason for hiding this comment

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

This should test for the absence of external

@ghost ghost force-pushed the polaris_truncate branch from b6a356a to 40263bc Compare August 11, 2021 17:19
@ghost ghost requested a review from voiid August 12, 2021 18:13
@ghost ghost merged commit 6a32163 into main Aug 16, 2021
@ghost ghost deleted the polaris_truncate branch August 16, 2021 20:11
BPScott pushed a commit that referenced this pull request Aug 17, 2021
…Button (#4327)

* Add test modernization to Truncate and UnstyledButton

* Add changes to unstyledbutton related to disabled and download
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant