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

Editor: Test that consecutive edits to the same attribute after saving are considered "persistent". #17917

Merged
merged 3 commits into from Nov 22, 2019

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Oct 11, 2019

Closes #17914

Description

This PR adds an E2E test to guard against the regression fixed by #17888.

How has this been tested?

It was verified that the test tests the desired behavior and passes.

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.
@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Oct 16, 2019

requested a review from mcsf

I'll give you a review, but the tests don't pass. 😬

Related, I'm not a fan of our assertIsDirty helper. It slows the tests down significantly, and when something fails the logs don't point to the actual failed expectation. I think in many cases we don't need such a heavy test.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Oct 16, 2019

I'll give you a review, but the tests don't pass. 😬

Interesting, they do pass locally every time.

Related, I'm not a fan of our assertIsDirty helper. It slows the tests down significantly, and when something fails the logs don't point to the actual failed expectation. I think in many cases we don't need such a heavy test.

Yeah, we should at least make it a Custom Matcher so that the right line is logged.

@epiqueras epiqueras force-pushed the add/consecutive-edits-after-save-tests branch from 194af5f to 200924c Nov 21, 2019
@epiqueras epiqueras force-pushed the add/consecutive-edits-after-save-tests branch from 296a3b5 to 679764a Nov 22, 2019
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 22, 2019

@mcsf I figured it out!

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 22, 2019

@mcsf I figured it out!

Thanks for coming back to this one! Unfortunately I still get failures, at least when run with:

PUPPETEER_SKIP_CHROMIUM_DOWNLOAD=true \
PUPPETEER_HEADLESS=false \
PUPPETEER_SLOWMO=80 \
npm run test-e2e -- \
  packages/e2e-tests/specs/editor/various/change-detection.test.js \
  -t 'consecutive edits to the same'

Output:

  ● Change detection › consecutive edits to the same attribute should mark the post as dirty after a save

    TimeoutError: waiting for selector ".editor-post-save-draft" failed: timeout 30000ms exceeded

      389 | 
      390 | 		// Check that the post is dirty.
    > 391 | 		await page.waitForSelector( '.editor-post-save-draft' );
          | 		           ^
      392 | 	} );
      393 | } );
      394 | 
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 22, 2019

Are you running the latest version of WP trunk? Because it's passing in Travis.

It sounds like you are using twentynineteen where the largest font size is called "Huge" instead of "Larger" so the selector misses it.

I tried changing the selectors and running your command on twentynineteen and it passed.

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 22, 2019

It sounds like you are using twentynineteen where the largest font size is called "Huge" instead of "Larger" so the selector misses it.

Ooh, yes, I forgot I was on Ninetween! Never mind. Yep, this seems to work!

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 22, 2019

It does make me think that tests should be able to define certain expectations, such as that the default theme is active.

@mcsf
mcsf approved these changes Nov 22, 2019
Copy link
Contributor

mcsf left a comment

Thanks!

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 22, 2019

@mcsf What do you think about adding a check in the testing script to verify the active theme before wastefully running the tests?

@epiqueras epiqueras merged commit 11d4cdc into master Nov 22, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras deleted the add/consecutive-edits-after-save-tests branch Nov 22, 2019
@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 22, 2019

@mcsf What do you think about adding a check in the testing script to verify the active theme before wastefully running the tests?

Could explore that, sure. I'm wary of harming performance by having checks that need to navigate to other parts of WP-Admin, though, so I wonder how acceptably we could just check for the presence of telltale elements like #twentytwenty-block-editor-styles-css. What do you think?

@epiqueras epiqueras mentioned this pull request Nov 22, 2019
6 of 6 tasks complete
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 22, 2019

I did something like that here: #18699.

It also only runs once before all tests so performance shouldn't be an issue.

@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.