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

Testing: Skip unreliable end-to-end tests #15059

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 18, 2019

This pull request seeks to disable unreliable end-to-end test cases until their reliability can be improved. They have been shown to cause intermittent build failures.

Testing Instructions:

Ensure end-to-end tests pass:

npm run build && npm run test-e2e

Follow-up Actions:

Create issue(s) for re-enabling skipped tests.

@aduth aduth added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Apr 18, 2019
@@ -30,7 +30,7 @@ describe( 'block deletion -', () => {
beforeEach( addThreeParagraphsToNewPost );

describe( 'deleting the third block using the Remove Block menu item', () => {
it( 'results in two remaining blocks and positions the caret at the end of the second block', async () => {
it.skip( 'results in two remaining blocks and positions the caret at the end of the second block', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The one I'm hesitant because in my experience at least it has shown to be unreliable only when using "popular plugins" and the fact that the popular plugins test didn't prove to be succesful IMO, I wonder if we should remove those instead.

@gziolo
Copy link
Member

gziolo commented Apr 18, 2019

I’m coming to the conclusion that we should skip all tests that will fail a given number of times on Travis in a week or similar period. Review all of them, open issues if those failures are legitimate and included test’s source code as an example. Once it’s done we should remove all skipped tests.

@aduth
Copy link
Member Author

aduth commented Apr 18, 2019

I’m coming to the conclusion that we should skip all tests that will fail a given number of times on Travis in a week or similar period. Review all of them, open issues if those failures are legitimate and included test’s source code as an example. Once it’s done we should remove all skipped tests.

Yes, I think this is all important, that (a) tests are reliable and (b) we commit to returning these to be unskipped.

The one I'm hesitant because in my experience at least it has shown to be unreliable only when using "popular plugins" and the fact that the popular plugins test didn't prove to be succesful IMO, I wonder if we should remove those instead.

I hesitate here because I'm sure to abandon the plugins testing altogether as these issues, when present, could likely be legitimate. But I agree that they make debugging more difficult, and haven't provided very much value overall.

To @gziolo 's point, I'm also considering to not put much thought into being bogged down to how specifically we decide to skip tests and just opt to skip indiscriminately, and evaluate options afterward.

I've spent some time debugging this specific failure. In doing so, I observe the tedium of debugging on behalf of these other popular plugins (and not knowing which specific one could be a culprit). One lead I have is in the fact that you can observe in standard use of the block settings menu, the "Convert to Reusable Block" button appears only after a short delay (because it fetches user permission to create reusable block). I wonder that there may be a race condition similar to what we've seen previously with animations and the difference in time between when Puppeteer calculates a position and when it enacts a specific interaction (#13739). In this case, I wonder if it calculates the position of "Remove Block", then clicks, but in the time between those two steps the "Add to Reusable Blocks" button appears in the exact location the "Remove Block" button had been. This could explain the mismatch in the snapshot where, for no discernible reason, a reusable block exists in the content.

      "<!-- wp:paragraph -->
      <p>First paragraph</p>
      <!-- /wp:paragraph -->
      
      <!-- wp:paragraph -->
    - <p>Second paragraph - caret was here</p>
    - <!-- /wp:paragraph -->"
    + <p>Second paragraph</p>
    + <!-- /wp:paragraph -->
    + 
    + <!-- wp:block {"ref":133} /-->"

@aduth
Copy link
Member Author

aduth commented Apr 18, 2019

the "Convert to Reusable Block" button appears only after a short delay (because it fetches user permission to create reusable block)

This could also explain why it only fails with the "popular plugins" enabled because popular plugins are enabled for the administrative user tests (who would have permission and thus the button would appear async) and not enabled for the author user tests (who would not have permission and thus the button would never appear). In further investigation, an author should have permission to create a reusable block, so this might not be a correct analysis.

gutenberg/.travis.yml

Lines 67 to 101 in 58da78c

- name: E2E tests (Admin with plugins) (1/2)
env: WP_VERSION=latest SCRIPT_DEBUG=false POPULAR_PLUGINS=true
install:
- ./bin/setup-local-env.sh
script:
- $( npm bin )/wp-scripts test-e2e --config=./packages/e2e-tests/jest.config.js --listTests > ~/.jest-e2e-tests
- npm run build
- npm run test-e2e -- --ci --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 0' < ~/.jest-e2e-tests )
- name: E2E tests (Admin with plugins) (2/2)
env: WP_VERSION=latest SCRIPT_DEBUG=false POPULAR_PLUGINS=true
install:
- ./bin/setup-local-env.sh
script:
- $( npm bin )/wp-scripts test-e2e --config=./packages/e2e-tests/jest.config.js --listTests > ~/.jest-e2e-tests
- npm run build
- npm run test-e2e -- --ci --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 1' < ~/.jest-e2e-tests )
- name: E2E tests (Author without plugins) (1/2)
env: WP_VERSION=latest SCRIPT_DEBUG=false E2E_ROLE=author
install:
- ./bin/setup-local-env.sh
script:
- $( npm bin )/wp-scripts test-e2e --config=./packages/e2e-tests/jest.config.js --listTests > ~/.jest-e2e-tests
- npm run build
- npm run test-e2e -- --ci --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 0' < ~/.jest-e2e-tests )
- name: E2E tests (Author without plugins) (2/2)
env: WP_VERSION=latest SCRIPT_DEBUG=false E2E_ROLE=author
install:
- ./bin/setup-local-env.sh
script:
- $( npm bin )/wp-scripts test-e2e --config=./packages/e2e-tests/jest.config.js --listTests > ~/.jest-e2e-tests
- npm run build
- npm run test-e2e -- --ci --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 2 == 1' < ~/.jest-e2e-tests )

@aduth
Copy link
Member Author

aduth commented Apr 18, 2019

you can observe in standard use of the block settings menu, the "Convert to Reusable Block" button appears only after a short delay (because it fetches user permission to create reusable block)

Turns out it's my fault. At one point (#12378) this was preloaded (and thus there'd be no delay), but the preloading was removed in #13569, possibly because it had already landed in Core trunk by that time (WordPress/wordpress-develop@79a3abc). Since this won't be reflected until WordPress 5.2, I've proposed it be re-added at #15061.

I don't consider it a blocker to merge this pull request. I can merge this one and then un-skip the test from #15061.

@aduth aduth merged commit 5d25504 into master Apr 18, 2019
@aduth aduth deleted the update/disable-e2e-intermittent-failures branch April 18, 2019 19:58
aduth added a commit that referenced this pull request Apr 18, 2019
daniloercoli added a commit that referenced this pull request Apr 19, 2019
…rnmobile/887-History-stack-is-not-empty-on-a-fresh-start-of-the-editor

* 'master' of https://github.com/WordPress/gutenberg:
  Avoid running hasMetaBoxes on each subscribe (#15041)
  Avoid passing down isFirst and isLast props (#15043)
  Add "Roadmap" document with an overview of projects in consideration. (#14907)
  Testing: Update tests to use Escape press to activate block toolbar (#15063)
  Testing: Skip unreliable end-to-end tests (#15059)
@gziolo
Copy link
Member

gziolo commented Apr 19, 2019

Thanks for working on this one. I think it's extremely important to make e2e tests reliable as it seems to be a major issue for new contributors. It might scare them off when they discover those failures. We take too much for granted because we are on the project for so long. We know that those failures happen and have permissions to re-trigger builds. It shouldn't be the case.

aduth added a commit that referenced this pull request Apr 24, 2019
* Reusable Blocks: Preload user permissions

* Testing: Unskip block deletion test

See #15059 (comment)
@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
@gziolo gziolo mentioned this pull request Oct 17, 2019
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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