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

Reset embed mocks on every request, stop request to instagram #15046

Merged
merged 2 commits into from
Apr 18, 2019

Conversation

notnownikki
Copy link
Member

@notnownikki notnownikki commented Apr 18, 2019

Description

After the retry test, mocks were not reset and so actual network requests happened. This made the tests unreliable, so this PR makes sure the mocks are reset for every test, and the instagram request is also mocked.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

// Respond to the instagram URL with a non-image response, doesn't matter what it is,
// just make sure the image errors.
{
match: createURLMatcher( 'https://www.instagram.com/p/Bvl97o2AK6x/' ),
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove the other matcher?

match: createEmbeddingMatcher( 'https://www.instagram.com/p/Bvl97o2AK6x/' ),
onRequestMatch: createJSONResponse( MOCK_EMBED_IMAGE_SUCCESS_RESPONSE ),

Copy link
Member

Choose a reason for hiding this comment

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

it doesn't seem to be used at the moment

Copy link
Member Author

Choose a reason for hiding this comment

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

No, one is to match the embed preview request for the URL and one is to match the URL itself. The image block creates a new image, and sets the URL as the source. If the image errors, it replaces itself with an appropriate embed block if there is one. So we need to mock the instagram URL, and the embed proxy call that generates the preview.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, it makes sense now :)

@gziolo gziolo added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Block] Embed Affects the Embed Block [Type] Bug An existing feature does not function as intended labels Apr 18, 2019
@gziolo gziolo added this to the 5.6 (Gutenberg) milestone Apr 18, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Many thanks for working on this fix. Let's get this in and see if it resolves the issue I have in #15018 :)

@gziolo gziolo merged commit 58da78c into master Apr 18, 2019
@gziolo gziolo deleted the fix/embedding-test-mocking branch April 18, 2019 14:00
@aduth
Copy link
Member

aduth commented Apr 18, 2019

I'm not positive that it's related to the changes here, though it appears relevant: In #15049 I've encountered an intermittent test failure from the embedding.test.js suite:

FAIL packages/e2e-tests/specs/embedding.test.js (65.496s)
  ● Embedding content › should transform from audio to embed block when Soundcloud URL is pasted
    waiting for selector ".wp-block-embed-soundcloud" failed: timeout 30000ms exceeded
      299 | 		await page.keyboard.type( 'https://soundcloud.com/a-boogie-wit-da-hoodie/swervin' );
      300 | 		await page.keyboard.press( 'Enter' );
    > 301 | 		await page.waitForSelector( '.wp-block-embed-soundcloud' );
          | 		           ^
      302 | 	} );
      303 | } );
      304 | 
      at new WaitTask (../../node_modules/puppeteer/lib/FrameManager.js:840:28)
      at Frame._waitForSelectorOrXPath (../../node_modules/puppeteer/lib/FrameManager.js:731:12)
      at Frame.waitForSelector (../../node_modules/puppeteer/lib/FrameManager.js:690:17)
      at Page.waitForSelector (../../node_modules/puppeteer/lib/Page.js:1008:29)
      at Object.waitForSelector (specs/embedding.test.js:301:14)
      at tryCatch (../../node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (../../node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (../../node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (specs/embedding.test.js:9:103)
      at _next (specs/embedding.test.js:11:194)

https://travis-ci.com/WordPress/gutenberg/jobs/194055234

@youknowriad
Copy link
Contributor

Got the same error in another PR @aduth I think we should start disabling all intermittent failures and create issues about these tests.

daniloercoli added a commit that referenced this pull request Apr 18, 2019
…rnmobile/887-History-stack-is-not-empty-on-a-fresh-start-of-the-editor

* 'master' of https://github.com/WordPress/gutenberg:
  Reset embed mocks on every request, stop request to instagram (#15046)
  Refactor core blocks to have save and transforms in their own files (part 2) (#14899)
  Fix pullquote import (#15036)
  Refactor core blocks to have save and transforms in their own files (part 4) (#14903)
  Refactor core blocks to have save and transforms in their own files (part 3) (#14902)
  Refactor core blocks to have deprecated extracted to their ownf files (p.1) (#14979)
  Test transform from media to embed blocks (#13997)
  If a more recent revision/autosave exists, store its state on editor setup (#7945)
  chore(release): publish
@notnownikki
Copy link
Member Author

Yes, the timeout failures aren't caused by the changes here, they're uncovered by the changes here. Previously the tests were failing most of the time with console errors due to the mocks not being applied.

I've seen these happen once running the tests locally, I'm not sure what causes it, but the end result is that the blocks aren't getting replaced with embed blocks when they should be. The failure I saw locally was the video block turned into a video player, trying to play a youtube URL.

I agree with disabling the tests, and I can see if I can track down what's causing it, although I doubt it's related to the embed blocks, as the problem is we don't get on to the stage where the embed blocks appear.

@aduth
Copy link
Member

aduth commented Apr 18, 2019

Got the same error in another PR @aduth I think we should start disabling all intermittent failures and create issues about these tests.

See #15059

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Embed Affects the Embed Block [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants