Skip to content
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

new driver for hint testing #2898

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from
Draft

Conversation

adids1221
Copy link
Contributor

@adids1221 adids1221 commented Jan 16, 2024

Description

Hint - create new driver and refactor tests to it

Changelog

Additional info

@ethanshar ethanshar added this to the Quality milestone Jan 22, 2024
@adids1221 adids1221 marked this pull request as ready for review January 22, 2024 09:04
jestSetup/jest-setup.js Outdated Show resolved Hide resolved
jestSetup/jest-setup.js Outdated Show resolved Hide resolved
Comment on lines 26 to 29
const hintTouchableDriver = usePressableDriver<TouchableOpacityProps>(useComponentDriver({
renderTree: props.renderTree,
testID: `${props.testID}`
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this just the Hint itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it is, do you think that the naming isn't good ?
It's the Touchable part of the component with out the View wrapper

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think it should be exported as press and probably something like pressOnBackground: overlayDriver.press for the overlay (same as in Modal)

src/components/hint/Hint.driver.new.ts Outdated Show resolved Hide resolved
src/components/hint/Hint.driver.new.ts Outdated Show resolved Hide resolved
src/components/hint/__tests__/index.spec.tsx Outdated Show resolved Hide resolved
src/components/hint/__tests__/index.spec.tsx Outdated Show resolved Hide resolved
src/components/hint/__tests__/index.spec.tsx Outdated Show resolved Hide resolved
src/components/hint/index.tsx Outdated Show resolved Hide resolved
src/components/hint/index.tsx Outdated Show resolved Hide resolved
@M-i-k-e-l
Copy link
Contributor

@adids1221 I'd also remove the comment from the change log 🙏

@M-i-k-e-l
Copy link
Contributor

Also, nice work on the mock, I'm sure that was not easy 👍

@adids1221
Copy link
Contributor Author

@M-i-k-e-l fix most of the comments, about the onBackgroundPress I'll try to add more test cases.
And about the mock I'll try to ask Niryo and I'll update.

@adids1221
Copy link
Contributor Author

@M-i-k-e-l didn't finished yet, having some issues with the Modal - isVisble in the test.
I'll ping you when all will be ready.

@adids1221
Copy link
Contributor Author

@M-i-k-e-l as we spoke we can't use isVisble on the Modal since it's not rendering to the screen.

@M-i-k-e-l
Copy link
Contributor

Hi @adids1221,
As discussed, please change the getOverlay and modalDriver (this is not the convention BTW) to specific getters

src/components/hint/Hint.driver.new.ts Outdated Show resolved Hide resolved
src/components/hint/Hint.driver.new.ts Outdated Show resolved Hide resolved
src/components/hint/Hint.driver.new.ts Outdated Show resolved Hide resolved
return overlayDriver;
};

const getIsModalVisible = () => {
Copy link
Contributor

@M-i-k-e-l M-i-k-e-l Apr 9, 2024

Choose a reason for hiding this comment

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

IMO there should be an isVisible for both modal and overlay

Can't really remember the reason for your previous comment about modal's visibility, lets discuss it if it's still relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping on this

@adids1221 adids1221 requested a review from M-i-k-e-l April 9, 2024 06:51
describe('Test Hint content', () => {
it('Hint should include message', async () => {
const renderTree = render(<HintTestComponent showHint/>);
const message = renderTree.getByTestId(`${TEST_ID}.message.text`).props.children;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be exported from the driver

const renderTree = render(<HintTestComponent showHint onBackgroundPress={() => {}}/>);
const driver = HintDriver({renderTree, testID: TEST_ID});
expect(await driver.exists()).toBeTruthy();
expect(await driver.isModalVisible()).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want this or maybe just a generic isOpen? WDYT?

isBackgroundExists,
pressOnBackground,
isModalVisible,
...driver,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the ...driver export, maybe export what is actually needed?

return modalDriver.isVisible();
};

const press = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

press is show? Or can it be both show and hide?

const expectedColor = Colors.$backgroundPrimaryHeavy;
const renderTree = render(<HintTestComponent showHint/>);
const driver = HintDriver({renderTree, testID: TEST_ID});
expect(await driver.getElement()).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'de change this to something else, what are we really testing here?

return {
getBackgroundColor,
getIcon,
isBackgroundExists,
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer isBackgroundVisible, WDYT?

return overlayDriver;
};

const getIsModalVisible = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ping on this

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Jul 30, 2024
@adids1221 adids1221 marked this pull request as draft September 5, 2024 13:40
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.

3 participants