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

Change Detection: Add a test case for post trashing. #18290

Merged
merged 4 commits into from
Nov 7, 2019

Conversation

epiqueras
Copy link
Contributor

Follows #18275

Description

This PR fixes and reintroduces the test case from #18275.

It also replaces some ad-hoc saving code with the correct e2e test util, saveDraft.

How has this been tested?

The new tests were verified to test for the issue fixed in #18275.

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.

@epiqueras epiqueras added [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Package] Editor /packages/editor labels Nov 5, 2019
@epiqueras epiqueras added this to the Future milestone Nov 5, 2019
@epiqueras epiqueras self-assigned this Nov 5, 2019
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

There's a (likely related) end-to-end test failure:

FAIL packages/e2e-tests/specs/editor/various/change-detection.test.js (60.563s)
  ● Change detection › should not prompt to confirm unsaved changes when trashing an existing post
    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 500 (Internal Server Error)"]
      at Object.expect (../jest-console/build/@wordpress/jest-console/src/index.js:34:4)
          at runMicrotasks (<anonymous>)

await page.waitForSelector( '.editor-post-saved-state.is-saved' );

// Check that the dialog didn't show.
await assertIsDirty( false );
Copy link
Member

Choose a reason for hiding this comment

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

From your comment at #18275 (comment) :

This works because refreshing a page with a dialog doesn't get rid of the dialog. So if navigation didn't happen because the dialog stopped it, assertIsDirty wouldn't assert false.

But the 'dialog' event handler isn't added until assertIsDirty is called, so if the dialog was already present by the time we reach this line, the event callback would never be triggered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True! I missed that.

@epiqueras
Copy link
Contributor Author

Fixed!

@epiqueras epiqueras requested a review from gziolo as a code owner November 6, 2019 18:26
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

@epiqueras epiqueras merged commit e1fb88e into master Nov 7, 2019
@epiqueras epiqueras deleted the add/change-detection-trashing-test-case branch November 7, 2019 22:51
@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.9 Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Editor /packages/editor [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