Skip to content

Blocks: Replace store name string with exposed store definition#27336

Merged
gziolo merged 7 commits intoWordPress:masterfrom
rafaelgallani:refactor/replace-blocks-store-key-with-store-def
Nov 30, 2020
Merged

Blocks: Replace store name string with exposed store definition#27336
gziolo merged 7 commits intoWordPress:masterfrom
rafaelgallani:refactor/replace-blocks-store-key-with-store-def

Conversation

@rafaelgallani
Copy link
Copy Markdown
Contributor

@rafaelgallani rafaelgallani commented Nov 27, 2020

Description

Addresses #27088. Replaces the core/blocks store name references (hardcoded strings) with the exposed store definitions. Had to change some core/blocks unit tests too, since they're testing the exposed definition now.

How has this been tested?

Just making sure npm run test wasn't failing.

Screenshots

blocks_unit_tests

Types of changes

Code refactoring, non-breaking change.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@rafaelgallani
Copy link
Copy Markdown
Contributor Author

rafaelgallani commented Nov 27, 2020 •
edited
Loading

That's weird... Apparently, the single test file that failed in the E2E tests runs fine here:

System information

system_information

e2e test screenshot

I will try triggering the checks once more with an empty commit...

@rafaelgallani
Copy link
Copy Markdown
Contributor Author

rafaelgallani commented Nov 27, 2020

It failed again. I tried rerunning the tests locally, apparently, it's all good:

e2e_2nd_try

As text: 
❯ $( npm bin )/wp-scripts test-e2e --config=./packages/e2e-tests/jest.config.js --cacheDirectory="$HOME/.jest-cache" --runTestsByPath $( awk 'NR % 4 == 2' < ~/.jest-e2e-tests )
 PASS  packages/e2e-tests/specs/local/demo.test.js (9.162s)
 PASS  packages/e2e-tests/specs/editor/various/writing-flow.test.js (117.21s)
 PASS  packages/e2e-tests/specs/editor/various/autosave.test.js (66.434s)
 PASS  packages/e2e-tests/specs/experiments/navigation.test.js (38.856s)
 PASS  packages/e2e-tests/specs/editor/plugins/plugins-api.test.js (37.439s)
 PASS  packages/e2e-tests/specs/editor/blocks/heading.test.js (32.808s)
 PASS  packages/e2e-tests/specs/experiments/multi-entity-editing.test.js (30.002s)
 PASS  packages/e2e-tests/specs/editor/various/preview.test.js (27.179s)
 PASS  packages/e2e-tests/specs/editor/various/sidebar.test.js (22.311s)
 PASS  packages/e2e-tests/specs/editor/plugins/container-blocks.test.js (21.492s)
 PASS  packages/e2e-tests/specs/editor/various/taxonomies.test.js (17.924s)
 PASS  packages/e2e-tests/specs/editor/blocks/classic.test.js (18.551s)
 PASS  packages/e2e-tests/specs/editor/plugins/inner-blocks-allowed-blocks.test.js (18.041s)
 PASS  packages/e2e-tests/specs/editor/various/publish-panel.test.js (13.283s)
 PASS  packages/e2e-tests/specs/experiments/multi-entity-saving.test.js (13.033s)
 PASS  packages/e2e-tests/specs/editor/various/nux.test.js (13.401s)
 PASS  packages/e2e-tests/specs/editor/various/navigable-toolbar.test.js (13.913s)
 PASS  packages/e2e-tests/specs/editor/various/duplicating-blocks.test.js (11.204s)
 PASS  packages/e2e-tests/specs/editor/blocks/spacer.test.js (9.35s)
 PASS  packages/e2e-tests/specs/editor/various/compatibility-classic-editor.test.js (9.759s)
 PASS  packages/e2e-tests/specs/editor/various/preferences.test.js (7.388s)
 PASS  packages/e2e-tests/specs/editor/blocks/cover.test.js (7.263s)
 PASS  packages/e2e-tests/specs/editor/blocks/columns.test.js (6.628s)
 PASS  packages/e2e-tests/specs/editor/plugins/custom-taxonomies.test.js (6.123s)
 PASS  packages/e2e-tests/specs/editor/various/adding-patterns.test.js (6.076s)
 PASS  packages/e2e-tests/specs/editor/various/fullscreen-mode.test.js (5.173s)

Test Suites: 26 passed, 26 total
Tests:       113 passed, 113 total
Snapshots:   61 passed, 61 total
Time:        580.458s

I'm not sure why they're failing in the checks...

@gziolo gziolo self-requested a review November 27, 2020 18:01
@gziolo gziolo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Blocks /packages/blocks labels Nov 27, 2020
@rafaelgallani
Copy link
Copy Markdown
Contributor Author

rafaelgallani commented Nov 28, 2020

I just tried:

  • Rebuilding everything using npm run wp-env destroy, rm -rf build, npm ci and npm run build;
  • Emulating slow CPU and internet connection as oriented here.

Same results, all tests passed.

I also checked manually one of the errors that happened in the build, and apparently, it's all good; the element exists:

e2e test element

I'm open to ideas. I have no clue what's happening. 😞

@gziolo
Copy link
Copy Markdown
Member

gziolo commented Nov 28, 2020

Feel free to ignore those failing e2e tests, they shouldn't be relevant to your changes. As far as I remember, there is PR that tries to make them more stable on CI.

@rafaelgallani
Copy link
Copy Markdown
Contributor Author

rafaelgallani commented Nov 28, 2020

Umm... Yeah. Sorry... I should've checked the E2E status before commenting/testing. Apparently, #27347 will fix it 😸

gziolo
gziolo approved these changes Nov 30, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Everything looks good, thank you for another great PR 👍

Comment thread packages/blocks/src/api/raw-handling/test/utils.js
select: jest.fn( ( store ) => {
switch ( store ) {
case 'core/blocks': {
case [ mockStoreName ]:
Copy link
Copy Markdown
Member

@gziolo gziolo Nov 30, 2020

Choose a reason for hiding this comment

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

Quite tricky, but it makes a lot of sense 👍

Copy link
Copy Markdown
Contributor Author

@rafaelgallani rafaelgallani Nov 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I could be replacing the switch statement to an if if that's better 😅.

Copy link
Copy Markdown
Member

@gziolo gziolo Nov 30, 2020

Choose a reason for hiding this comment

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

I was commenting about the whole process of mocking. It is it what it is, we better leave it 😃

@gziolo gziolo mentioned this pull request Nov 30, 2020
Closed
17 tasks
@gziolo gziolo merged commit 2239c5d into WordPress:master Nov 30, 2020
@github-actions github-actions Bot added this to the Gutenberg 9.5 milestone Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gziolo gziolo gziolo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@ellatrix ellatrix Awaiting requested review from ellatrix

@talldan talldan Awaiting requested review from talldan

Assignees

No one assigned

Labels

[Package] Blocks /packages/blocks [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Milestone

Gutenberg 9.5

Development

Successfully merging this pull request may close these issues.

2 participants

@rafaelgallani @gziolo