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

Testing: Add E2E test to verify demo errors #9924

Merged
merged 4 commits into from Sep 21, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Sep 14, 2018

See: #9599 (comment), #9500 (comment)

This pull request seeks to add a basic end-to-end test which visits the Demo page. This will help surface issues such as those described above where demo content is invalid due to block deprecations. It achieves this by leveraging existing behaviors to surface any console logging which occurs during the E2E test run as an error. However, it required some minor changes to the @wordpress/jest-console package to support this use. Those changes are included.

Testing instructions:

Verify there are no block validation errors in the editor block list or developer tools console when navigating to Gutenberg > Demo.

Ensure end-to-end tests pass:

npm run test-e2e

Ensure unit tests for @wordpress/jest-console pass:

npm run test-unit packages/jest-console

If you're feeling adventurous, revert c6008fb in your local copy of the branch and verify that end-to-end tests fail.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 14, 2018

Member

Hah, funnily enough I think the changes to improve jest-console incidentally helped capture some unrelated-but-legitimate-looking errors:


  ● new editor state › should not error
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Failed to load resource: the server responded with a status of 403 (Forbidden)"],["Error: <svg> attribute preserveAspectRatio: Trailing garbage, \"xMinYMin none\"."]
      32 | 	function assertExpectedCalls() {
      33 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 34 | 			expect( console ).not[ matcherName ]();
         | 			^
      35 | 		}
      36 | 	}
      37 |
      at assertExpectedCalls (packages/jest-console/src/index.js:34:4)
      at Object.<anonymous> (packages/jest-console/src/index.js:41:3)
PASS test/e2e/specs/a11y.test.js
Summary of all failing tests
FAIL test/e2e/specs/demo.test.js
  ● new editor state › should not error
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Failed to load resource: the server responded with a status of 403 (Forbidden)"],["Error: <svg> attribute preserveAspectRatio: Trailing garbage, \"xMinYMin none\"."]
      32 | 	function assertExpectedCalls() {
      33 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 34 | 			expect( console ).not[ matcherName ]();
         | 			^
      35 | 		}
      36 | 	}
      37 |
      at assertExpectedCalls (packages/jest-console/src/index.js:34:4)
      at Object.<anonymous> (packages/jest-console/src/index.js:41:3)

Will need to investigate further on Monday.

Member

aduth commented Sep 14, 2018

Hah, funnily enough I think the changes to improve jest-console incidentally helped capture some unrelated-but-legitimate-looking errors:


  ● new editor state › should not error
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Failed to load resource: the server responded with a status of 403 (Forbidden)"],["Error: <svg> attribute preserveAspectRatio: Trailing garbage, \"xMinYMin none\"."]
      32 | 	function assertExpectedCalls() {
      33 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 34 | 			expect( console ).not[ matcherName ]();
         | 			^
      35 | 		}
      36 | 	}
      37 |
      at assertExpectedCalls (packages/jest-console/src/index.js:34:4)
      at Object.<anonymous> (packages/jest-console/src/index.js:41:3)
PASS test/e2e/specs/a11y.test.js
Summary of all failing tests
FAIL test/e2e/specs/demo.test.js
  ● new editor state › should not error
    expect(jest.fn()).not.toHaveErrored(expected)
    Expected mock function not to be called but it was called with:
    ["Failed to load resource: the server responded with a status of 403 (Forbidden)"],["Error: <svg> attribute preserveAspectRatio: Trailing garbage, \"xMinYMin none\"."]
      32 | 	function assertExpectedCalls() {
      33 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 34 | 			expect( console ).not[ matcherName ]();
         | 			^
      35 | 		}
      36 | 	}
      37 |
      at assertExpectedCalls (packages/jest-console/src/index.js:34:4)
      at Object.<anonymous> (packages/jest-console/src/index.js:41:3)

Will need to investigate further on Monday.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 17, 2018

Member

This is a tough one to debug, and I'm not able to reproduce locally. I suspect there's something going on specifically within the Travis Docker process preventing some network requests from occurring unimpaired. The error message is quite cryptic too.

Member

aduth commented Sep 17, 2018

This is a tough one to debug, and I'm not able to reproduce locally. I suspect there's something going on specifically within the Travis Docker process preventing some network requests from occurring unimpaired. The error message is quite cryptic too.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 18, 2018

Member

The culprit was the Vimeo embed. It's doing some request, apparently for an SVG resource, that is either getting mangled or blocked by the Travis setup. My suggestion would be to use different embedded content, or to override it somehow during the test so that embeds still get tested as part of the demo page, but the problem with Travis + Vimeo doesn't show up.

Member

notnownikki commented Sep 18, 2018

The culprit was the Vimeo embed. It's doing some request, apparently for an SVG resource, that is either getting mangled or blocked by the Travis setup. My suggestion would be to use different embedded content, or to override it somehow during the test so that embeds still get tested as part of the demo page, but the problem with Travis + Vimeo doesn't show up.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 18, 2018

Member

to override it somehow during the test so that embeds still get tested as part of the demo page, but the problem with Travis + Vimeo doesn't show up.

Yeah, it seems reasonable to me that we should want to exempt from our generalized console error capturing anything which is logged as an error as part of an embedded frame. If it's possible to distinguish, that is.

Member

aduth commented Sep 18, 2018

to override it somehow during the test so that embeds still get tested as part of the demo page, but the problem with Travis + Vimeo doesn't show up.

Yeah, it seems reasonable to me that we should want to exempt from our generalized console error capturing anything which is logged as an error as part of an embedded frame. If it's possible to distinguish, that is.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 18, 2018

Member

I've only just started to read the docs, but it seems like it's possible to intercept the requests made, so it might be possible to intercept the requests made to external sites and respond with a mock request that always succeeds.

Member

notnownikki commented Sep 18, 2018

I've only just started to read the docs, but it seems like it's possible to intercept the requests made, so it might be possible to intercept the requests made to external sites and respond with a mock request that always succeeds.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 18, 2018

Member

To some degree, we should want visibility even in those cases, at least when its within our control to remedy. For a Vimeo iframe which is producing errors, however, there's not much we can do aside from either omitting it (either from the demo content, or altogether), which is where I think allowing some exceptions (targeted at iframe content) could be reasonable.

Member

aduth commented Sep 18, 2018

To some degree, we should want visibility even in those cases, at least when its within our control to remedy. For a Vimeo iframe which is producing errors, however, there's not much we can do aside from either omitting it (either from the demo content, or altogether), which is where I think allowing some exceptions (targeted at iframe content) could be reasonable.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 18, 2018

Member

I think we have to look at excluding errors that come from the iframe, because of the restrictions on that iframe there are a number of things blocked. For instance, Twitter submits a form, for tracking purposes I think, and that's blocked and gets an error in the iframe.

Member

notnownikki commented Sep 18, 2018

I think we have to look at excluding errors that come from the iframe, because of the restrictions on that iframe there are a number of things blocked. For instance, Twitter submits a form, for tracking purposes I think, and that's blocked and gets an error in the iframe.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 18, 2018

Member

We might consider swapping page.on( 'console' ) by overriding the window.console global instead. It's not entirely clear by the documentation, but I imagine the former will capture any console logging, where the latter might only reflect logging on the top-level window.

page.on( 'console', ( message ) => {

https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#event-console

Alternatively, we could override the console.log on each frame to behave as a noop, though it's not clear how we'd implement this as iframes can be added or removed from the page on demand (MutationObserver would probably be the non-perfect solution here).

Member

aduth commented Sep 18, 2018

We might consider swapping page.on( 'console' ) by overriding the window.console global instead. It's not entirely clear by the documentation, but I imagine the former will capture any console logging, where the latter might only reflect logging on the top-level window.

page.on( 'console', ( message ) => {

https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#event-console

Alternatively, we could override the console.log on each frame to behave as a noop, though it's not clear how we'd implement this as iframes can be added or removed from the page on demand (MutationObserver would probably be the non-perfect solution here).

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 19, 2018

Member

The more I think about this, the more I think that we don't want third party code running during the test. I can't see a way that would reliably let us differentiate between errors resulting from embedded code, and actual errors in the embed block.

So my suggestion is that we use request interception to mock the embed call, and return known HTML. https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#event-request and https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pagesetrequestinterceptionvalue seem to be good starting points on that.

Member

notnownikki commented Sep 19, 2018

The more I think about this, the more I think that we don't want third party code running during the test. I can't see a way that would reliably let us differentiate between errors resulting from embedded code, and actual errors in the embed block.

So my suggestion is that we use request interception to mock the embed call, and return known HTML. https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#event-request and https://github.com/GoogleChrome/puppeteer/blob/master/docs/api.md#pagesetrequestinterceptionvalue seem to be good starting points on that.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 19, 2018

Member

I can't see a way that would reliably let us differentiate between errors resulting from embedded code, and actual errors in the embed block.

So my suggestion is that we use request interception to mock the embed call, and return known HTML.

That sounds fine to me, assuming we can pretty well isolate these sources of "foreign" markup (I'm imagining it's all getting funneled through the oEmbed endpoint).

Member

aduth commented Sep 19, 2018

I can't see a way that would reliably let us differentiate between errors resulting from embedded code, and actual errors in the embed block.

So my suggestion is that we use request interception to mock the embed call, and return known HTML.

That sounds fine to me, assuming we can pretty well isolate these sources of "foreign" markup (I'm imagining it's all getting funneled through the oEmbed endpoint).

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 19, 2018

Member

(I'm imagining it's all getting funneled through the oEmbed endpoint).

It is :)

Member

notnownikki commented Sep 19, 2018

(I'm imagining it's all getting funneled through the oEmbed endpoint).

It is :)

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 20, 2018

Member

I'm going to push some test commits up today to see if the request interception works.

Member

notnownikki commented Sep 20, 2018

I'm going to push some test commits up today to see if the request interception works.

@notnownikki

This comment has been minimized.

Show comment
Hide comment
@notnownikki

notnownikki Sep 20, 2018

Member

...and we're passing!

The test caught a couple of blocks that needed updating in the demo content. The request interception works, and is returning the same response as the Vimeo embed, but with different HTML so it doesn't result in any external requests.

Member

notnownikki commented Sep 20, 2018

...and we're passing!

The test caught a couple of blocks that needed updating in the demo content. The request interception works, and is returning the same response as the Vimeo embed, but with different HTML so it doesn't result in any external requests.

@notnownikki

This is a great addition, and caught two actual errors when I merged master so that it would run locally :)

On an unrelated note, splitting-merging.test.js is intermittently failing, so we should look at that in another PR.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 21, 2018

Member

Nice work tracking down and fixing the issue @notnownikki !

For a follow-up, I like this idea that we mock any foreign embed content, and think we should apply it more generally: for all third party embeds, and for all of our test cases (i.e. configured in test setup).

Member

aduth commented Sep 21, 2018

Nice work tracking down and fixing the issue @notnownikki !

For a follow-up, I like this idea that we mock any foreign embed content, and think we should apply it more generally: for all third party embeds, and for all of our test cases (i.e. configured in test setup).

@aduth aduth merged commit b4c8ffc into master Sep 21, 2018

2 checks passed

codecov/project 48.81% (+0.01%) compared to 2cd23ca
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@aduth aduth deleted the add/e2e-demo branch Sep 21, 2018

@mcsf mcsf referenced this pull request Sep 21, 2018

Merged

Demo: Update demo content (embed) to avoid automatic update #10098

0 of 4 tasks complete

@mtias mtias added this to the 4.0 milestone Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment