aryella-lacerda / react-native-accessibility-engine

Make accessibility-related assertions on React Native code using React Test Renderer
MIT License
165 stars 13 forks source link

Rules & other options are not configurable in the jest test matcher #310

Closed emilyseibert closed 1 year ago

emilyseibert commented 1 year ago

Describe the ~bug~ feature Currently the engine code has option capabilities, but those are only accessible in rule tests & users of this library are forced to run with all rules enabled. My proposal (draft PR) is to enable these options to configure the behavior of the jest matcher.

We're super excited to use this library within our codebase, but we have large amounts of code that are not accessible. My hope is that we can support the gradual adoption of these a11y rules within our code base by configuring which rules are enabled while running .toBeAccessible. Like eslint, developers can configure rules to enable/disable coverage as they adopt in large legacy repos.

expect(button).toBeAccessible({ruleIds: ['no-empty-text']});

Additionally, I've added an optional configuration that can generically override the jest matcher violation handling. This would be another option to enable adoption. For example, repos can codemod to create .toBeAccessible tests, but only throw warnings / console logs for tests that are failing.

const overrideFunction = (violations) => {
    console.warn('A11y Violations', violations);    
    return [];
}

expect(button).toBeAccessible({overrideReturnFunctionality: overrideFunction});

I've already created the PR (a little out of order 😅 ) to help demonstrate use cases and to hopefully contribute forward! Please let me know what you think, and I'm open to feedback!

Context

aryella-lacerda commented 1 year ago

Hi, @emilyseibert! I think this looks awesome. 😁 No two development teams are dealing with the same context, so this gives the users more control over how they use the lib. A few thoughts:

Global options

Passing an options object to each instance of the .toBeAccessible() matcher will probably be a hassle in larger code bases. Could we make it possible to configure the engine globally? In this case, passing options to specific instances of the matcher would just override the global configs for that test.

The returnViolations option

Currently, it doesn't really make sense to support all the engine's options directly through the matcher. For example:

BUT... I think it SHOULD make sense to support all the engine's options directly through the matcher.

The only thing you'd have to do to make this possible is remove the returnViolations option, which is true by default and isn't currently exposed to the user. The engine would always return an array of violations and this if-statement wouldn't need to exist.

The ruleIds option

I think we can keep the name of this option the same (rules) and just change its value from an array of rule objects to an array of rule ids. It'll make for a cleaner API that way.

The overrideReturnFunctionality option

I like the implementation, it's just the name that isn't very clear. 🤔 Maybe:

I especially like this option because it allows users to suppress errors in third-party libraries, for example.

emilyseibert commented 1 year ago

@aryella-lacerda Thank you for the suggestions! I've made your suggested changes and set my PR (#309 ) as "Ready to review". Can you take a look? Thanks :)

aryella-lacerda commented 1 year ago

PR merged! Just released version v3.2.0