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

Improve openAllBlockInserterCategories function; #14460

Conversation

jorgefilipecosta
Copy link
Member

Fix intermittent failures in inner-blocks-allowed-blocks.test.js.

How has this been tested?

The best method seems to be executing this test in Travis CI multiple times and check that the tests don't fail.

…lures in inner-blocks-allowed-blocks.test.js.
@jorgefilipecosta jorgefilipecosta force-pushed the fix/intermitent-failures-in-inner-blocks-allowed-blocks.test.js branch from 30e82f4 to 7556bf2 Compare March 15, 2019 17:09
@afercia
Copy link
Contributor

afercia commented Mar 16, 2019

To my understanding, for some reasons the name of this branch fix/intermitent-failures-in-inner-blocks-allowed-blocks.test.js (notice the part blocks.test.js) makes npm test fail locally for me. Seems the .git folder is not excluded 😬 and npm test tries to run it as it was a test file. Console output:

Summary of all failing tests
 FAIL  .git/logs/refs/remotes/origin/fix/intermitent-failures-in-inner-blocks-allowed-blocks.test.js
  ● Test suite failed to run

    SyntaxError: /Users/andrea/wp/plugins/gutenberg/.git/logs/refs/remotes/origin/fix/intermitent-failures-in-inner-blocks-allowed-blocks.test.js: Legacy octal literals are not allowed in strict mode (1:0)

    etc...

 FAIL  .git/refs/remotes/origin/fix/intermitent-failures-in-inner-blocks-allowed-blocks.test.js
  ● Test suite failed to run

    SyntaxError: /Users/andrea/wp/plugins/gutenberg/.git/refs/remotes/origin/fix/intermitent-failures-in-inner-blocks-allowed-blocks.test.js: Identifier directly after number (1:4)

    etc...

@gziolo gziolo added the [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. label Mar 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.

I can confirm that it solves the original issue. I'm not 100% confident about using while loop but maybe it's okay given that Jest sets a timeout for each test.

await categoryPanel.click();
categoryPanel = await page.$( notOppenedCategorySelector );
Copy link
Member

Choose a reason for hiding this comment

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

How about we keep the for of loop and we wait until it has is-opened class applied? I never feel confident about using while loop :)
I'm not sure to be honest, maybe this is okay since it has to await and would timeout anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this approach would still be problematic, because if we used the selector '.block-editor-inserter__results .components-panel__body' in a for of, as soon as we applied the first click, all the other selectors would be invalid, because they reference an element that no longer exists on the dom because of a rerender that happens on click so we would not be able to use the element to wait for a class.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it's a complex stuff :)

@jorgefilipecosta jorgefilipecosta added this to the 5.3 (Gutenberg) milestone Mar 18, 2019
@jorgefilipecosta jorgefilipecosta merged commit 15b20c7 into master Mar 18, 2019
@jorgefilipecosta jorgefilipecosta deleted the fix/intermitent-failures-in-inner-blocks-allowed-blocks.test.js branch March 18, 2019 08:37
@jorgefilipecosta
Copy link
Member Author

Merging this so we can delete the branch and avoid problems like the one referred by @afercia. If anyone has more thoughts on this I will gladly iterate.
Thank you for sharing this problem @afercia I will avoid branch with names like this in the future, and I will look into a way to exclude the git folder.

youknowriad pushed a commit that referenced this pull request Mar 20, 2019
youknowriad pushed a commit that referenced this pull request Mar 20, 2019
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