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(matchers): add expect(locator).toContainClass() #14219

Closed
wants to merge 13 commits into from

Conversation

Diokuz
Copy link

@Diokuz Diokuz commented May 17, 2022

@ghost
Copy link

ghost commented May 17, 2022

CLA assistant check
All CLA requirements met.

@Diokuz Diokuz force-pushed the main branch 4 times, most recently from 44cd222 to 04e2b47 Compare May 18, 2022 06:53
@Diokuz Diokuz force-pushed the main branch 2 times, most recently from a731e09 to be98ef4 Compare May 18, 2022 11:41
Copy link
Member

@mxschmitt mxschmitt left a comment

Choose a reason for hiding this comment

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

very nice, requires some polishing in the docs (examples of the other languages) then it should be good to go!

docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
tests/playwright-test/playwright.expect.misc.spec.ts Outdated Show resolved Hide resolved
tests/playwright-test/playwright.expect.misc.spec.ts Outdated Show resolved Hide resolved
Diokuz and others added 8 commits May 18, 2022 17:11
Co-authored-by: Max Schmitt <max@schmitt.mx>
Co-authored-by: Max Schmitt <max@schmitt.mx>
Co-authored-by: Max Schmitt <max@schmitt.mx>
Co-authored-by: Max Schmitt <max@schmitt.mx>
Co-authored-by: Max Schmitt <max@schmitt.mx>
Co-authored-by: Max Schmitt <max@schmitt.mx>
@Diokuz
Copy link
Author

Diokuz commented May 19, 2022

@mxschmitt looks like I fixed everything you comment. What next?

docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
docs/src/api/class-locatorassertions.md Outdated Show resolved Hide resolved
await expect(itemLocator).toContainClass(['alice', 'bob', 'carl']); // fail, length mismatch
```

```java
Copy link
Member

Choose a reason for hiding this comment

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

All languages should have equivalent examples. You can make JS snippet smaller or other languages bigger.

Copy link
Author

Choose a reason for hiding this comment

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

I just copypasted docs for toHaveClass and renamed it :)

docs/src/api/class-locatorassertions.md Show resolved Hide resolved
@@ -1297,9 +1297,9 @@ export class Frame extends SdkObject {
omitAttached = true;
else if (options.isNot && options.expression === 'to.have.count' && options.expectedNumber !== 0)
omitAttached = true;
else if (!options.isNot && options.expression.endsWith('.array') && options.expectedText!.length === 0)
else if (!options.isNot && options.expression.endsWith('.array') && options.expectedText?.length === 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

Copy link
Author

Choose a reason for hiding this comment

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

Because expectedText could be undefined, and it is in my case.

omitAttached = true;
else if (options.isNot && options.expression.endsWith('.array') && options.expectedText!.length > 0)
else if (options.isNot && options.expression.endsWith('.array') && (options.expectedText?.length || 0) > 0)
Copy link
Member

Choose a reason for hiding this comment

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

Why did this change?

if (expression === 'to.contain.class.array') {
const receivedClassNames = elements.map(e => e.className);

if (!Array.isArray(options.expectedValue) || options.expectedValue.length !== elements.length)
Copy link
Member

Choose a reason for hiding this comment

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

This does not match the logic of to.contain.text.array, toContainClass relative to toHaveClass should be the same as toContainText relative to toHaveText, let's make sure they are consistent.

Copy link
Author

Choose a reason for hiding this comment

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

Could you please give me a testcase for what you are saying? This condition just ensures that elements and values length are equal.

Diokuz and others added 4 commits May 28, 2022 22:18
Co-authored-by: Pavel Feldman <pavel.feldman@gmail.com>
Co-authored-by: Pavel Feldman <pavel.feldman@gmail.com>
Co-authored-by: Pavel Feldman <pavel.feldman@gmail.com>
Co-authored-by: Pavel Feldman <pavel.feldman@gmail.com>
@Diokuz
Copy link
Author

Diokuz commented Jun 6, 2022

So, guys, I think this mr on your side now) I'll continue to watch for #14117

@mxschmitt
Copy link
Member

Thank you for your work @Diokuz, I merged via #15491.

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