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

fix(html-reporter): add filter for anonymous describe #30621

Merged

Conversation

dev-jonghoonpark
Copy link
Contributor

related issue: #30475

Motivation:

On #30475, we found that anonymous describe is rendered in html report

Modification:

Make filter for anonymous describe

Result:

anonymous describe will be filtered out.
Not render empty describe
Close #30475 issue

@dev-jonghoonpark dev-jonghoonpark force-pushed the fix/add-anonymous-describe-filter branch from 061bd98 to c9bd2eb Compare May 1, 2024 13:15

This comment has been minimized.

This comment has been minimized.

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.

Thank you for the PR! It looks good, but let's move the fix from the UI towards the report generation.

Comment on lines 2359 to 2360
const testFilePathLinkTitle = await testFilePathLink.getAttribute('title');
expect(testFilePathLinkTitle).toEqual('Root describe › Test passed');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const testFilePathLinkTitle = await testFilePathLink.getAttribute('title');
expect(testFilePathLinkTitle).toEqual('Root describe › Test passed');
await expect(testFilePathLink).toHaveText('Root describe › Test passed');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is for validating the title attribute of the a tag.
It's not caught by toHaveText.

Is it possible to validate attributes with toHaveText?
I tried it just in case and got an error.

Here's an example

<a class="test-file-path-link" href="#?testId=0ae68002180e7361ff85-5ea21f3c6f68eb98910d" title="Root describe › Test passed" style="text-decoration: none; color: var(--color-fg-default);">
    <span class="test-file-path">anonymous-describe.test.ts:6</span>
</a>

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, I missed the previous check for the text. We strongly prefer toHaveText() over toHaveCount(1). To check the title, you can use toHaveAttribute(). We also don't like CSS locators like .hbox a[title="..."], so I'd just drop that check. Overall, something like this would be a good test:

await showReport();

await expect(page.locator('.test-file-test')).toHaveCount(1);

const testFilePathLink = page.locator('.test-file-path-link');
await expect(testFilePathLink).toHaveText('Root describe › Test passed');
await expect(testFilePathLink).toHaveAttribute('title', 'Root describe › Test passed');

await testFilePathLink.click();
await expect(page.locator('.test-case-path')).toHaveText('Root describe');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgozman

Thanks for the answer
I replaced that part with toHaveAttribute.

But .hbox a[title="..."] would be hard to replace.
because the first a tag doesn't have a class.

The html code for one row looks like this ('Root describe › Test passed' appears three times.)

<div class="test-file-test test-file-test-outcome-expected">
    <div class="hbox" style="align-items: flex-start;">
        <div class="hbox">
            <span class="test-file-test-status-icon">
                <svg>...</svg>
            </span>
            <span>
                <a class="" href="#?testId=0ae68002180e7361ff85-5ea21f3c6f68eb98910d" title="Root describe › Test passed" style="text-decoration: none; color: var(--color-fg-default);">
                    <span class="test-file-title">Root describe › Test passed</span>
                </a>
            </span>
        </div>
        <span data-testid="test-duration" style="min-width: 50px; text-align: right;">1.7s</span>
    </div>
    <div class="test-file-details-row">
        <a class="test-file-path-link" href="#?testId=0ae68002180e7361ff85-5ea21f3c6f68eb98910d" title="Root describe › Test passed" style="text-decoration: none; color: var(--color-fg-default);">
            <span class="test-file-path">anonymous-describe.test.ts:6</span>
        </a>
        <a class="test-file-badge" href="trace/index.html?trace=http://localhost:9323/data/41f3e012e4cd5dfb0adbef872a709158450a441e.zip" title="View trace" style="text-decoration: none; color: var(--color-fg-default);">
            <svg>...</svg>
        </a>
    </div>
</div>

So I changed it to the following
Can you check if that's okay?

await showReport();

await expect(page.locator('.test-file-test')).toHaveCount(1);

await expect(page.locator('.test-file-test').locator('a').first()).toHaveAttribute('title', 'Root describe › Test passed');
await expect(page.locator('.test-file-title')).toHaveText('Root describe › Test passed');
const testFilePathLink = page.locator('.test-file-path-link');
await expect(testFilePathLink).toHaveAttribute('title', 'Root describe › Test passed');

await testFilePathLink.click();
await expect(page.locator('.test-case-path')).toHaveText('Root describe');

packages/html-reporter/src/testFileView.tsx Outdated Show resolved Hide resolved
@dev-jonghoonpark dev-jonghoonpark force-pushed the fix/add-anonymous-describe-filter branch from c9bd2eb to e1a939a Compare May 2, 2024 02:01

This comment has been minimized.

@dev-jonghoonpark dev-jonghoonpark force-pushed the fix/add-anonymous-describe-filter branch from e1a939a to bf7716a Compare May 2, 2024 16:02
@dev-jonghoonpark dev-jonghoonpark force-pushed the fix/add-anonymous-describe-filter branch from bf7716a to 13e2a6d Compare May 2, 2024 16:18

This comment has been minimized.

Copy link
Contributor

github-actions bot commented May 2, 2024

Test results for "tests 1"

27289 passed, 671 skipped
✔️✔️✔️

Merge workflow run.

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 good, merging in. Thank you for the PR!

@dgozman dgozman merged commit a6488c4 into microsoft:main May 2, 2024
30 checks passed
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.

[Bug]: Anonymous describe is rendered in HTML report
2 participants