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

Try using axe disableFrame function to resolve Axe end to end test error #26535

Closed
wants to merge 8 commits into from

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Oct 28, 2020

Description

Currently axe core tests are failing intermittently on this test:

Template Part › Template part placeholder › Should insert template part when preview is selected

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Failed to inject axe-core into frame (about:blank)"],["Failed to inject axe-core into frame (about:blank)"]

There's an error injecting axe core scripts into iframes created by the block preview.

#26527 tried to use the exclude option to make axe ignore block previews, but Axe still tries to inject the script.

Axe has a disableFrame function that looks like it prevents axe from inject scripts into specified iframes. Its undocumented (dequelabs/axe-core-npm#39), so not sure how we'd feel about using it.

This PR tests it out.

@talldan talldan added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Oct 28, 2020
@talldan talldan self-assigned this Oct 28, 2020
@talldan talldan changed the title Try using axe disableFrame function Try using axe disableFrame function to resolve Axe end to end test error Oct 28, 2020
@github-actions
Copy link

github-actions bot commented Oct 28, 2020

Size Change: +1.48 kB (0%)

Total Size: 1.21 MB

Filename Size Change
build/block-directory/index.js 8.72 kB -2 B (0%)
build/block-editor/index.js 130 kB +7 B (0%)
build/block-library/editor-rtl.css 8.98 kB +35 B (0%)
build/block-library/editor.css 8.98 kB +37 B (0%)
build/block-library/index.js 146 kB +609 B (0%)
build/block-library/style-rtl.css 7.83 kB +64 B (0%)
build/block-library/style.css 7.84 kB +69 B (0%)
build/blocks/index.js 48.1 kB +1 B
build/components/index.js 172 kB +435 B (0%)
build/components/style-rtl.css 15.2 kB -120 B (0%)
build/components/style.css 15.2 kB -120 B (0%)
build/data/index.js 8.77 kB +2 B (0%)
build/edit-post/style-rtl.css 6.38 kB +17 B (0%)
build/edit-post/style.css 6.37 kB +16 B (0%)
build/edit-site/index.js 22.1 kB +273 B (1%)
build/edit-site/style-rtl.css 3.86 kB +61 B (1%)
build/edit-site/style.css 3.85 kB +61 B (1%)
build/edit-widgets/index.js 26.4 kB +2 B (0%)
build/edit-widgets/style-rtl.css 3.1 kB +16 B (0%)
build/edit-widgets/style.css 3.1 kB +16 B (0%)
build/redux-routine/index.js 2.85 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/theme-rtl.css 837 B 0 B
build/block-library/theme.css 838 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 772 B 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

There's an error injecting axe core scripts into iframes created by the block preview.

It would explain why it fails also for the demo test:

expect( await page.$( 'button.editor-post-save-draft' ) ).toBeTruthy();

It triggers the preview as well. The way to go would be rather to find a way to ensure that the page that is about to be validated contains the block editor. We surely don't want to validate about:blank page.

In theory, we check whether there is a block editor on the page here and bail out when the condition is not met:

if ( ! ( await page.$( '.block-editor' ) ) ) {
return;
}

It looks like tests should wait until the preview is loaded before it finished as it probably happens that at the time axe starts processing navigation between pages takes effect.

@talldan
Copy link
Contributor Author

talldan commented Oct 28, 2020

It triggers the preview as well.

@gziolo I didn't realise that. I thought it just loaded the demo and then checked the dirty state.

@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

It triggers the preview as well.

@gziolo I didn't realise that. I thought it just loaded the demo and then checked the dirty state.

Ah, wait. I misinterpreted this code. Anyway, something goes wrong there as well. So maybe there is something strange happening that creates iframes that are empty? It might be also an issue with axe itself, since we upgraded to v4.

@talldan
Copy link
Contributor Author

talldan commented Oct 28, 2020

The stack trace does seem to indicate it's caused by axe. The embed mocking in that test creates an empty iframe, so that could be related?

The other thought I had is that it doesn't like the multiple visitAdminPage calls, they're causing an unexpected navigation, but then visitAdminPage works pretty well elsewhere.

@talldan
Copy link
Contributor Author

talldan commented Oct 28, 2020

Well, all checks have passed twice in a row now, which seems like more success than we've had lately?

I might just want to double-check that I haven't disabled axe tests entirely 😅

@gziolo gziolo added the [Tool] Jest Puppeteer aXe /packages/jest-puppeteer-axe label Oct 28, 2020
@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

Well, all checks have passed twice in a row now, which seems like more success than we've had lately?

I might just want to double-check that I haven't disabled axe tests entirely 😅

Let's make sure that those axe tests still verify enabled rules.

How about this new option is marked as experimental for the start so we could validate if it solves the issue? This way we won't need to document it inside @wordpress/jest-puppeteer-axe until we are sure this API is solid. By the way, this package is used in the Storybook addon (https://www.npmjs.com/package/@storybook/addon-storyshots-puppeteer).

@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

I commented out some skipped axe rules to see if axe still works. Feel free to remove 01eff11 once we know :)

@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

There are valid failures reported which means that everything works perfectly fine with the changes proposed. I'm happy to approve this PR when disabledFrames is updated to __experimentalDisabledFrames and my test commit is removed.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

This proposed solution with the disabledFrames looks good to me! Also supporting the __experimental recommendation.

Comment on lines 196 to 204
'button-name',
'color-contrast',
'dlitem',
// 'button-name',
// 'color-contrast',
// 'dlitem',
'duplicate-id',
'label',
'landmark-one-main',
// 'label',
// 'landmark-one-main',
'link-name',
'listitem',
'region',
// 'listitem',
// 'region',
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Oct 28, 2020

Choose a reason for hiding this comment

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

I might have missed it in the discussion. Was there a reason we are commenting out a handful of the disabled rules?

nevermind i see it in comment - #26535 (comment) - is the idea to re-enable (or re-disable 🤣 ) these before merging?

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

Also worth noting #26527 seems to be passing now (3x in a row at least) that I updated the selector for block previews from the container to the content. So that might still make sense as an alternative (or addition) to this?

Its a bit less 'experimental' in nature than what we are doing here with the undocumented disableFrame, but I agree the approach here also makes sense and that disabling everything under [aria-hidden="true"] seems like it should be done in general for axe tests. 🤔

@talldan
Copy link
Contributor Author

talldan commented Oct 29, 2020

Closing in favor of #26527. We can always come back to this if the problem reoccurs.

edit: Reopening as #26527 doesn't quite seem to have made it go away.

@talldan talldan closed this Oct 29, 2020
@talldan talldan deleted the try/disabling-preview-iframes-in-axe-tests branch October 29, 2020 05:29
@talldan talldan restored the try/disabling-preview-iframes-in-axe-tests branch October 30, 2020 03:09
@talldan talldan reopened this Oct 30, 2020
@talldan
Copy link
Contributor Author

talldan commented Oct 30, 2020

There are valid failures reported which means that everything works perfectly fine with the changes proposed. I'm happy to approve this PR when disabledFrames is updated to __experimentalDisabledFrames and my test commit is removed.

Agree it should be experimental for now. I've made those changes.

@talldan
Copy link
Contributor Author

talldan commented Oct 30, 2020

Ah, well on the last run this also just failed with the same iframe error:

FAIL specs/experiments/template-part.test.js (21.914s)
  ● Template Part › Template part placeholder › Should insert template part when preview is selected

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Failed to inject axe-core into frame (about:blank)"],["Failed to inject axe-core into frame (about:blank)"]

So I guess this won't solve the issue either.

@talldan
Copy link
Contributor Author

talldan commented Oct 30, 2020

Another alternative, we can ignore the logged message - #26590

@talldan talldan closed this Nov 2, 2020
@talldan talldan deleted the try/disabling-preview-iframes-in-axe-tests branch November 2, 2020 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Tool] Jest Puppeteer aXe /packages/jest-puppeteer-axe [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants