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

[BUG] toHaveAttribute always passes when given a blank value string #16517

Closed
radium-v opened this issue Aug 12, 2022 · 4 comments · Fixed by #16767
Closed

[BUG] toHaveAttribute always passes when given a blank value string #16517

radium-v opened this issue Aug 12, 2022 · 4 comments · Fixed by #16767

Comments

@radium-v
Copy link

radium-v commented Aug 12, 2022

Context:

  • Playwright Version: 1.25
  • Operating System: Windows 11, Ubuntu 22.04 via WSL2
  • Node.js version: 16.16.0
  • Browser: All

Code Snippet

import { expect, test } from "@playwright/test";

test("should return false for nonexistent attributes", async ({ page }) => {
    await page.setContent(`<div></div>`);

    const element = page.locator("div");

    // always passes
    await expect(element).toHaveAttribute("", "");

    // always passes
    await expect(element).toHaveAttribute("any-arbitrary-attribute", "");

    // always fails, `value` must be a string
    await expect(element).not.toHaveAttribute("any-attribute", "");

    // always fails (Matcher error: expected value must be a string or regular expression)
    await expect(element).not.toHaveAttribute("attr");

    // workaround: passes when the attribute is not present
    expect(await element.getAttribute("attr")).toBeNull();

    await element.evaluate(node => node.toggleAttribute("attr"));

    // workaround: this solution checks for the existence of boolean attributes with no value, but doesn't cover every valid boolean attribute situation
    expect(await element.getAttribute("attr")).toBe("");
});

Describe the bug

The toHaveAttribute matcher doesn't work as expected with the checks above, since await expect(element).toHaveAttribute("anything", "") will always pass. Likewise, await expect(element).not.toHaveAttribute("anything", "") will always fail for any attribute when used as a check for the presence of attributes. In our case, this primarily affects the use of boolean attributes like checked, disabled, hidden, open, which are considered true when they're present, regardless of value.

I think the value parameter for toHaveAttribute could be made optional, which would allow checking for the existence of attributes. The always-failing not.toHaveAttribute matcher would then be able to pass. This may also help to address #16270, though boolean attributes likely would need their own logic to match the allowed validity rules described in this MDN article.

@aslushnikov
Copy link
Collaborator

I think the value parameter for toHaveAttribute could be made optional, which would allow checking for the existence of attributes

@radium-v Sounds legit to me, let me bring it up on the team meeting.

@aslushnikov
Copy link
Collaborator

@radium-v workaround: could you please try with empty slashes? 🤷‍♂️

await expect(element).toHaveAttribute('any-arbitrary-attribute', /.*/);

Other than that, we discussed the feature and it sounds legit! We'll implement it

@aslushnikov aslushnikov self-assigned this Aug 15, 2022
@radium-v
Copy link
Author

I just tried these:

await expect(element).toHaveAttribute("some-attr", /.*/);
await expect(element).toHaveAttribute("some-attr", /^$/);

It seems the regex is comparing against empty string ("") which means they still incorrectly pass even when the attribute doesn't exist on the element.

@radium-v
Copy link
Author

As workarounds, I wrote these extensions:

  1. For the existence of attributes regardless of the value ([Feature] Add locator.hasAttribute() #16270 (comment)):

    expect.extend({
        async hasAttribute(recieved: Locator, attribute: string) {
            const pass = await recieved.evaluate(
                (node, attribute) => node.hasAttribute(attribute),
                attribute
            );
    
            return {
                message: () => `expected ${recieved} to have attribute \`${attribute}\``,
                pass,
            };
        },
    });
  2. For boolean attributes in particular:

    expect.extend({
        async toHaveBooleanAttribute(recieved: Locator, name: string) {
            name = name.trim();
    
            const [hasAttr, value] = await recieved.evaluate((node, name) => {
                if (!node.hasAttribute(name)) {
                    return [false, null];
                }
    
                return [true, node.getAttribute(name)];
            }, name);
    
            if (!hasAttr) {
                return {
                    message: () => `expected ${name} to exist on ${recieved.toString()}`,
                    pass: false,
                };
            }
    
            if (!this.isNot && value !== "" && value !== name) {
                return {
                    message: () =>
                        `expected attribute \`${name}\` to have a value of \`''\` or \`${name}\` on ${recieved}`,
                    pass: false,
                };
            }
    
            return {
                message: () => `expected ${recieved} to have boolean attribute ${name}`,
                pass: true,
            };
        },
    });

aslushnikov added a commit to aslushnikov/playwright that referenced this issue Aug 23, 2022
This patch changes `expect(locator).toHaveAttribute()` so that the
`value` argument can be omitted. When done so, the method will
assert attribute existance.

Fixes microsoft#16517
aslushnikov added a commit that referenced this issue Aug 25, 2022
…16767)

This patch changes `expect(locator).toHaveAttribute()` so that the
`value` argument can be omitted. When done so, the method will
assert attribute existance.

Fixes #16517
rwoll added a commit that referenced this issue Sep 16, 2022
Relates #16517.

Revert "docs(python): add missing NotToHaveAttribute overloads (#17371)"

This reverts commit 2e1ea29.

Revert "docs(release-notes): add 1.26 release notes for language ports
(#17345)"

This reverts commit 4b8a85e.

Revert "test: unflake "should support boolean attribute with options"
(#17024)"

This reverts commit 1dc05bd.

Revert "fix: support toHaveAttribute(name, options) (#16941)"

This reverts commit f30ac1d.

Revert "feat: expect(locator).toHaveAttribute to assert attribute
presence (#16767)"

This reverts commit 622c73c.
yury-s added a commit that referenced this issue Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants