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

feat(aria): extend toHaveAccessibleName() to accept an array of expected accessible names #33277

Merged
merged 8 commits into from
Nov 18, 2024

Conversation

aairiian
Copy link
Contributor

@aairiian aairiian commented Oct 24, 2024

extend toHaveAccessibleName() to accept an array of expected accessible names and write the tests for the type

Closes #32593.

…ted accessible names

This commit extends the toHaveAccessibleName() matcher to support an array

References microsoft#32593
This commit includes the handling accessible.name.array

References microsoft#32593
The expected string can be asserting softly without being equivalent to the actual string

References microsoft#32593

This comment has been minimized.

@aairiian
Copy link
Contributor Author

@aairiian please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

@aairiian aairiian changed the title [Feature]: Extend toHaveAccessibleName() to accept an array of expected accessible names #32593 feat(aria): extend toHaveAccessibleName() to accept an array of expected accessible names #32593 Oct 25, 2024

This comment has been minimized.

This comment has been minimized.

Copy link
Member

@Skn0tt Skn0tt left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, this looks great! I'll discuss with the team tonight to see if there's anything we need to pay close attention to.

}, expected, options);
if (Array.isArray(expected)) {
return toEqual.call(this, 'toHaveAccessibleName', locator, 'Locator', async (isNot, timeout) => {
const expectedText = serializeExpectedTextValues(expected, { matchSubstring: true, normalizeWhiteSpace: true, ignoreCase: options.ignoreCase });
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer the changes to normalizeWhiteSpace and matchSubstring to be in a separate PR, and discuss them in an issue first - especially the matchSubstring one sounds breaking to me.

Let's keep this PR strictly about accepting an array.

Copy link
Member

Choose a reason for hiding this comment

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

+1, let's only land the array aspect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I excluded the matchSubstring option from the string serialization

@Skn0tt
Copy link
Member

Skn0tt commented Oct 29, 2024

I'll discuss with the team tonight.

We're happy to go forward with this. We have a related feature in the works that's about ARIA snapshots, and that should be strictly better than the array forms of toMatchText, toHaveAccessibleName et al. So just to let you know - if we end up shipping that in this or one of the following releases, we'll drop this PR in favour of that.

@aairiian
Copy link
Contributor Author

I'll discuss with the team tonight.

We're happy to go forward with this. We have a related feature in the works that's about ARIA snapshots, and that should be strictly better than the array forms of toMatchText, toHaveAccessibleName et al. So just to let you know - if we end up shipping that in this or one of the following releases, we'll drop this PR in favour of that.

Thank you for getting back to me. I appreciate the time you've taken to review my PR and consider it 😌🙏🏻

@@ -1236,7 +1236,7 @@ await Expect(locator).toHaveAccessibleNameAsync("Save to disk");

### param: LocatorAssertions.toHaveAccessibleName.name
* since: v1.44
- `name` <[string]|[RegExp]>
- `name` <[string]|[RegExp]|[Array]<[string]>|[Array]<[RegExp]>>
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 be [Array]<[string]|[RegExp]>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the type format

}, expected, options);
if (Array.isArray(expected)) {
return toEqual.call(this, 'toHaveAccessibleName', locator, 'Locator', async (isNot, timeout) => {
const expectedText = serializeExpectedTextValues(expected, { matchSubstring: true, normalizeWhiteSpace: true, ignoreCase: options.ignoreCase });
Copy link
Member

Choose a reason for hiding this comment

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

+1, let's only land the array aspect.

This comment has been minimized.

Changed type accessible name in the doc file

References microsoft#32593
Copy link
Contributor

Test results for "tests 1"

2 flaky ⚠️ [installation tests] › playwright-electron-should-work.spec.ts:44:5 › should work when wrapped inside @playwright/test and trace is enabled @package-installations-macos-latest
⚠️ [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

36917 passed, 650 skipped
✔️✔️✔️

Merge workflow run.

@dgozman dgozman changed the title feat(aria): extend toHaveAccessibleName() to accept an array of expected accessible names #32593 feat(aria): extend toHaveAccessibleName() to accept an array of expected accessible names Nov 18, 2024
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for the PR!

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.

[Feature]: Extend toHaveAccessibleName() to accept an array of expected accessible names
4 participants