feat: add toBeVisible matcher #73

fluiddot · 2022-01-24T17:52:30Z

What:

Add toBeVisible matcher following a similar logic as the jest-dom matcher (reference).

Why:

This matcher provides a useful way to verify that a queried element is actually visible.

How:

The matcher has been introduced based on the implementation of other matchers and trying to reproduce a similar logic as the jest-dom matcher (reference), but taking into account the React Native nuances.

Checklist:

Documentation added to the
docs
Typescript definitions updated
Tests
Ready to be merged

dcalhoun

Great work! This matcher would be quite useful. I left a few comments to consider.

README.md

src/__tests__/to-be-visible.js

src/to-be-visible.js

fluiddot · 2022-01-25T10:36:13Z

Great work! This matcher would be quite useful. I left a few comments to consider.

Thank you very much @dcalhoun for taking a look at this PR 🙇.

Following your comments and suggestions, I've applied some updates to the changes. At this point, the PR can be considered as ready to be reviewed in order to include the new toBeVisible matcher.

fluiddot · 2022-01-25T12:58:59Z

Looks like most of the unit tests are failing with the following error:

TypeError: _interopRequireDefault is not a function

However, I managed to reproduce the same issue by running the tests in the main branch, so not sure whether these changes might be the cause 🤔 .

dcalhoun · 2022-06-18T21:19:32Z

@mdjastrzebski @thymikee would either of you be willing to review this work please? Totally understand if you do not have time currently. Just wanted to check in. Thanks! 🙇🏻

GuglielmoFelici · 2022-08-12T10:42:31Z

Hi all! Any update on this? Would be quite useful 🙌

mdjastrzebski · 2022-08-12T13:02:26Z

I will try to work on that next week

Norfeldt · 2022-08-15T09:32:52Z

I will try to work on that next week

Don't know if this would be of any help @mdjastrzebski . I'm using this:

expect.extend({
  // should be replacable once this PR gets merged https://github.com/testing-library/jest-native/pull/73
  toHaveZeroOpacity(element) {
    let pass = element.props.style?.opacity === 0
    let parent = element.parent
    while (parent && pass === false) {
      if (parent.props.style?.opacity === 0) pass = true
      parent = parent?.parent
    }

    return {
      message: () => 'expected element to be hidden',
      pass,
    }
  },
})

import { render } from '@testing-library/react-native'
import * as React from 'react'
import { View, Text } from 'react-native'

describe('extended `expect` methods', () => {
  describe('.toHaveZeroOpacity', () => {
    it('should pass when element is has zero opacity', () => {
      const utils = render(<View style={{ opacity: 0 }} testID="TestId__VIEW" />)
      expect(utils.getByTestId('TestId__VIEW')).toHaveZeroOpacity()
    })

    it('should NOT pass when element is has non-zero opacity', () => {
      const utils = render(<View style={{ opacity: 0.001 }} testID="TestId__VIEW" />)
      expect(utils.getByTestId('TestId__VIEW')).not.toHaveZeroOpacity()
    })

    it('should pass when some parent element is has zero opacity', () => {
      const utils = render(
        <View style={{ opacity: 0 }}>
          <View>
            <Text>invisible</Text>
          </View>
        </View>
      )
      expect(utils.getByText(/invisible/i)).toHaveZeroOpacity()
    })

    it('should pass when NO parent elements has zero opacity', () => {
      const utils = render(
        <View>
          <View>
            <Text>invisible</Text>
          </View>
        </View>
      )
      expect(utils.getByText(/invisible/i)).not.toHaveZeroOpacity()
    })
  })
})

mdjastrzebski

@fluiddot, @dcalhoun I've added some comments regarding sharing logic with a related PR in RNTL. We also removed ramda in the mean time so pls update your imports.

README.md

mdjastrzebski · 2022-08-17T13:13:28Z

src/to-be-visible.js

+}
+
+function isAttributeVisible(element) {
+  return element.type !== 'Modal' || element.props.visible !== false;


Directly checking modal type looks kinda hacky, maybe we could check some accessiblity props here: accessibilityElementsHidden, etc

does it support bottom sheet from gorhom ?

Hard to say without checking, depends on how well that library supports accessibility props. I think we should first concentrate on supporting built-in components, e.g. built-in Modal from RN, and handle the external libraries afterwards on a case-by-case basis, as in some cases the proper place to support them will be here, but in other case it might be by submitting the PR to the library authors to improve their accessibility support.

@fluiddot I wonder how Modal actually performs hidding of its children when visibility=false. Instead of relying on querying the element for type and visible prop, we could check the underlying code. Maybe it just does not render the children at all.

Instead of relying on querying the element for type and visible prop, we could check the underlying code. Maybe it just does not render the children at all.

@mdjastrzebski I checked when testing that Modal component doesn't actually render children, so this matcher will throw an exception when using it for children elements of a not visible modal.

Directly checking modal type looks kinda hacky, maybe we could check some accessiblity props here: accessibilityElementsHidden, etc

@mdjastrzebski good point. To be honest, I added the modal type check as a special case, since it's likely that we want to check the visibility of a modal using this matcher. In any case, I'm open to reconsider this condition in favor of a better approach or even not including it.

Regarding the accessibility props, I went ahead and updated the matcher to also check them in this commit, similar to the logic of isInaccessible function.

tomwaitforitmy · 2022-09-01T16:25:07Z

I would be very happy to use this! 👍

mdjastrzebski · 2022-09-30T12:18:23Z

@fluiddot Do you have capacity to work on updating this PR?

@dcalhoun, @Norfeldt & others: maybe you are willing to take over this.

re isInaccessible function/respectAccessiblity option from RNTL: it seems that to toBeVisible from Jest DOM has slightly different logic than isInaccessible from DTL, as it e.g. takes into account opacity: 0. So this matcher should not sole rely on isInaccessible.

fluiddot · 2022-09-30T16:46:01Z

@fluiddot Do you have capacity to work on updating this PR?

Hey @mdjastrzebski, my apologies for not being very active in the PR. I'd like to resume the work on this and review the feedback as soon as possible, but my bandwidth nowadays is usually low. If someone is willing to take over this, please feel to do it. Otherwise, I'll try to make some progress in the next weeks.

mdjastrzebski · 2022-10-01T13:19:49Z

src/__tests__/to-be-visible.js

+import { render } from '@testing-library/react-native';
+
+describe('.toBeVisible', () => {
+  test.each([


Github/link seems to be complaining about missing key prop, Could you refactor these tests to be regular. not .each tests for readability.

I fixed this issue when migrating the test file to TypeScript in this commit.

mdjastrzebski · 2022-10-01T13:22:22Z

src/to-be-visible.js

+
+function isStyleVisible(element) {
+  const style = element.props.style || {};
+  const { display = 'flex', opacity = 1 } = Array.isArray(style) ? mergeAll(style) : style;


Use StyleSheet.flatten to calculate flat style object.

Good catch, I've applied this suggestion in this commit.

mdjastrzebski · 2022-10-01T13:22:58Z

src/to-be-visible.js

+
+function isStyleVisible(element) {
+  const style = element.props.style || {};
+  const { display = 'flex', opacity = 1 } = Array.isArray(style) ? mergeAll(style) : style;


No need for default values, if we later compare them to known values.

Suggested change

const { display = 'flex', opacity = 1 } = Array.isArray(style) ? mergeAll(style) : style;

const { display, opacity } = Array.isArray(style) ? mergeAll(style) : style;

Agreed, I've applied this suggestion in this commit.

mdjastrzebski · 2022-10-01T13:26:19Z

@fluiddot Overall the current implementation looks solid. I think it's sensible approach to base it on Jest DOM matcher code with some tweaks to adjust for different props in RN than in Web.

# Conflicts: # extend-expect.d.ts # src/extend-expect.ts

mdjastrzebski · 2022-10-17T08:11:16Z

@fluiddot would you be able to rebase the code and migrate it to typescript as the current codebase uses it now.

Co-authored-by: Maciej Jastrzebski <[email protected]>

fluiddot · 2022-10-18T18:12:51Z

@fluiddot would you be able to rebase the code and migrate it to typescript as the current codebase uses it now.

@mdjastrzebski 😅 Sorry for the long delay on addressing the comments of the PR and migrating the changes to TypeScript. After the recent changes I pushed, the PR should be ready for another review. Let me know if there's anything else I can contribute with in order to conclude the feature. Thank you very much for the help 🙇 .

codecov · 2022-10-18T18:16:41Z

Codecov Report

Merging #73 (7267da4) into main (beaf547) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #73   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         9    +1     
  Lines          151       175   +24     
  Branches        47        57   +10     
=========================================
+ Hits           151       175   +24

Flag	Coverage Δ
node-14	`100.00% <100.00%> (ø)`
node-16	`100.00% <100.00%> (ø)`
node-18	`100.00% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/extend-expect.ts	`100.00% <ø> (ø)`
src/to-be-visible.ts	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

README.md

src/__tests__/to-be-visible.tsx

mdjastrzebski · 2022-10-20T12:19:49Z

@fluiddot looks pretty solid. I think the code is ready, I've added some tweak to code comments. Pls fix code coverage back to 100%. It should be a matter of adding a single test probably.

Co-authored-by: Maciej Jastrzebski <[email protected]>

fluiddot · 2022-10-20T18:34:34Z

@fluiddot looks pretty solid. I think the code is ready, I've added some tweak to code comments. Pls fix code coverage back to 100%. It should be a matter of adding a single test probably.

Thanks for the review @mdjastrzebski 🙇. I've just applied your suggestions and typo fixes. I'll take a look at the code coverage and try to add another test to provide 100% 👍.

…into feat/to-be-visible

fluiddot · 2022-10-20T18:58:19Z

@fluiddot looks pretty solid. I think the code is ready, I've added some tweak to code comments. Pls fix code coverage back to 100%. It should be a matter of adding a single test probably.

Thanks for the review @mdjastrzebski 🙇. I've just applied your suggestions and typo fixes. I'll take a look at the code coverage and try to add another test to provide 100% 👍.

I added a new test for checking that the matcher throws an error when the expectation is not matched. With this, the coverage should go back to 100% 🎊.

mdjastrzebski

Looks good! Happy to finally merge it :-) Thank you @fluiddot for this contribution!

github-actions · 2022-10-20T20:19:25Z

🎉 This PR is included in version 5.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

fluiddot · 2022-10-20T20:30:40Z

Looks good! Happy to finally merge it :-) Thank you @fluiddot for this contribution!

Thank you so much @mdjastrzebski for the review and help of this contribution 🙇! Happy to know that it's finally merged 😃!

fluiddot force-pushed the feat/to-be-visible branch from 48b38ae to 9fd8ec3 Compare January 24, 2022 18:12

This was referenced Jan 24, 2022

[RNMobile] Refactor react-native-editor initialization test WordPress/gutenberg#37955

Merged

Add a way to test Modal components #49

Closed

dcalhoun reviewed Jan 24, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/__tests__/to-be-visible.js Outdated Show resolved Hide resolved

src/to-be-visible.js Outdated Show resolved Hide resolved

fluiddot force-pushed the feat/to-be-visible branch 5 times, most recently from e7afe06 to 17eaad4 Compare January 25, 2022 10:26

fluiddot marked this pull request as ready for review January 25, 2022 10:28

fluiddot mentioned this pull request Jan 26, 2022

[RNMobile] Add custom matcher toBeVisible WordPress/gutenberg#38254

Merged

8 tasks

feat: add toBeVisible matcher

d81bc01

fluiddot force-pushed the feat/to-be-visible branch from 17eaad4 to d81bc01 Compare February 7, 2022 15:00

Merge branch 'main' into feat/to-be-visible

e9537e8

mdjastrzebski requested changes Aug 17, 2022

View reviewed changes

mdjastrzebski mentioned this pull request Sep 19, 2022

feat: isInaccessible API form RTL/DTL callstack/react-native-testing-library#1128

Merged

6 tasks

mdjastrzebski reviewed Oct 1, 2022

View reviewed changes

Merge branch 'main' into feat/to-be-visible

3a3e4a4

# Conflicts: # extend-expect.d.ts # src/extend-expect.ts

refactor: migrate to-be-visible to TypeScript

2335a11

fluiddot and others added 3 commits October 18, 2022 19:42

test: update to-be-visible test cases

861cc72

feat(toBeVisible): check if visible for accessibility

3348212

Update toBeVisible description

6c3bc55

Co-authored-by: Maciej Jastrzebski <[email protected]>

fluiddot requested a review from mdjastrzebski October 18, 2022 18:06