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

Command/40 Added checkBlockPatternExists command. #62

Merged
merged 22 commits into from
Nov 16, 2022
Merged

Command/40 Added checkBlockPatternExists command. #62

merged 22 commits into from
Nov 16, 2022

Conversation

faisal-alvi
Copy link
Member

Description of the Change

Added a new command checkBlockPatternExists which checks whether a given pattern exists on the site or not.

It accepts the following parameters.

  • title: Pattern name/title.
  • categoryValue: Value of the pattern category. for example "twentytwentyone" for the Twenty Twenty-One category.

Closes #40

Verification Process

  1. Build the project using npm run build.
  2. Make sure the docker is running.
  3. Run npm run env:start.
  4. Run npm run cypress:open.
  5. Run check-block-pattern-exists.test.js.
  6. Try to change the pattern name and its category in the /tests/cypress/integration/check-block-pattern-exists.test.js file and see the test results.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

Added - New Command checkBlockPatternExists to check if block pattern exists.

Credits

Props @faisal-alvi

@faisal-alvi faisal-alvi added this to the Future Release milestone Apr 27, 2022
@faisal-alvi faisal-alvi requested a review from a team April 27, 2022 12:27
@faisal-alvi faisal-alvi self-assigned this Apr 27, 2022
@faisal-alvi faisal-alvi requested review from Sidsector9 and removed request for a team April 27, 2022 12:27
@faisal-alvi faisal-alvi linked an issue Apr 27, 2022 that may be closed by this pull request
3 tasks
Copy link
Contributor

@cadic cadic left a comment

Choose a reason for hiding this comment

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

@faisal-alvi thank you for the great tool. I left few comments to the code.

It also seems to me that command name and/or the work it does is not accurate. The command will insert the block pattern if it finds it, while from its name we expect just the check of existence.

In case we just want to check if block pattern exists, maybe it's better to finish the procedure with wrapping the result. It will allow to chain and tester will decide if they want to insert the pattern:

cy
  .checkBlockPatternExists({title:'Title', categoryValue:'cat'})
  .then((exists) => {
    if(exists) {
      // insert pattern
    } else {
      // alternative test workflow
    }
  });

Please refer to createTerm command and it's tests to see how it works

cy.visit('/wp-admin/post-new.php');

// Close Welcome Guide.
const welcomeGuide =
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please update this to use closeWelcomeGuide command

}
});

// Open inerter.
Copy link
Contributor

Choose a reason for hiding this comment

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

A typo here?

const inserterWPNew = '.edit-post-header-toolbar__inserter-toggle';
cy.get('body').then($body => {
if ($body.find(inserterWPOld).length > 0) {
cy.get(inserterWPOld).click({ force: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

Using {force: true} brings a potentially false positive workflow to the test process. In case if the element is hidden, the command will click it anyway, while regular users could not do that.

While this behaviour is ok for testing end-products, but it's not welcomed within the library. @dinhtungdu, need your opinion on this please.

Copy link
Contributor

Choose a reason for hiding this comment

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

While this behaviour is ok for testing end-products, but it's not welcomed within the library.

@cadic I agree. Using click with force set to true will bypass actionability checks. In the other words, a hidden element can be clickable. It doesn't replicate human behavior so we should avoid that when possible.

@dinhtungdu
Copy link
Contributor

It also seems to me that command name and/or the work it does is not accurate. The command will insert the block pattern if it finds it, while from its name we expect just the check of existence.

I have the same thought. Besides, I think it's developer's responsibility to ensure the patterns exist before inserting. So we just need a command to simply insert a pattern to the page. If the pattern doesn't exist for any reason:

  • That's the potential issue of the plugin and should be fixed in the plugin source code.
  • Or that can be an issue with the test environment, which also shouldn't be covered by this library but the environment config instead.

@faisal-alvi
Copy link
Member Author

The cypress checks for the pattern but when we switch the category as shown in the below screenshot, the patterns render slowly (replaces is-placeholder divs one-by-one) and cause the test to fail. Tried using setInterval but that does not work well. Finally, as a workaround, added 10s of waiting time to allow WordPress to render patterns.

cy-40

Please share if there is a better solution.

@faisal-alvi faisal-alvi requested a review from cadic May 16, 2022 15:21
@Sidsector9
Copy link
Member

If the sole purpose of this command is to check whether a pattern exists or not, rather than going through the UI, can we use code instead?

Something like this:

cy.window().then( win => {
    const { wp } = win;

    const { getSettings } = wp.data.select( 'core/block-editor' );
    const allRegisteredPatterns = getSettings().__experimentalBlockPatterns;

    /**
     * loop through all the patterns to check if pattern exists
     */
} );

@Sidsector9
Copy link
Member

The function can be reduced to:

/* eslint-disable tsdoc/syntax */
/**
 * Check Block Pattern Exists. Works only with WordPress \>=5.5
 *
 * \@param postData {
 *          `title` - Patttern name/title,
 *          `categoryValue` - Value of the pattern category,
 *        }
 *
 * @example
 * For WP v5.5
 * ```
 * cy.checkBlockPatternExists({
 *    title: 'Two buttons',
 * });
 * ```
 *
 * @example
 * For WP v5.9
 * ```
 * cy.checkBlockPatternExists({
 *    title: 'Three columns with offset images',
 *    categoryValue: 'gallery',
 * });
 * ```
 */
export const checkBlockPatternExists = ({
  title,
  categoryValue = 'featured',
}: {
  title: string;
  categoryValue?: string;
}): void => {
  cy.visit('/wp-admin/post-new.php');

  // Close Welcome Guide.
  cy.closeWelcomeGuide();

  cy.window().then( win => {
    const { wp } = win;

    const { getSettings } = wp.data.select( 'core/block-editor' );
    const allRegisteredPatterns = getSettings().__experimentalBlockPatterns;

    for ( let i = 0; i < allRegisteredPatterns.length; i++ ) {
      if ( title === allRegisteredPatterns[i].title && allRegisteredPatterns[i].categories && allRegisteredPatterns[i].categories.includes(categoryValue) ) {
        cy.wrap(true);
        return;
      }
    }

    cy.wrap(false);
  });
};

@faisal-alvi
Copy link
Member Author

@Sidsector9 this is what I was thinking about but as the Cypress is an E2E test, I thought to stick with the UI.

I've checked the code and it works in old and new WP versions, however, while building the project, the following error is displayed:

image

@Sidsector9
Copy link
Member

@faisal-alvi I haven't looked through the project entirely, but adding the following at the top of your file should work:

declare global {
  interface Window {
    wp: any;
  }
}

this is what I was thinking about but as the Cypress is an E2E test, I thought to stick with the UI.

While you're correct to do so, one important thing to consider is that these utilities are not meant for testing Gutenberg itself. They're helpers to test "things" built using Gutenberg's features. Keeping this in mind, going the UI/non-UI way doesn't make much of a difference. @dinhtungdu, any thoughts on this?

@faisal-alvi faisal-alvi requested a review from dinhtungdu May 18, 2022 09:43
Copy link
Contributor

@dinhtungdu dinhtungdu left a comment

Choose a reason for hiding this comment

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

@faisal-alvi Can you please check and fix the canceled workflows here?

I agree with Sid that this doesn't make much difference for this command even in the E2E context.

@cadic
Copy link
Contributor

cadic commented May 22, 2022

@dinhtungdu looks like all 3 workflows were cancelled by timeout after 6 hours of being stuck

@faisal-alvi
Copy link
Member Author

The Github actions behave weirdly sometimes. I have tested in a local for WP 5.9 and the tests are passing:

cypress-util-pr-62

Any feedback from anyone? or are we good to go for merge?

Comment on lines 12 to 16
if (exists) {
alert('The block patter exists!');
} else {
alert('The block pattern does not exist!');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@faisal-alvi alert() requires human interaction to close the alert which never happens in the CI environments. I believe this causes the tests to be stuck.

@faisal-alvi
Copy link
Member Author

Awesome @dinhtungdu !!

@faisal-alvi faisal-alvi requested a review from dinhtungdu June 3, 2022 09:58
@cadic
Copy link
Contributor

cadic commented Jun 3, 2022

@faisal-alvi I've updated the test and checked the command against every WP core since 5.2

You can use run-all-cores.sh script for that:

./run-all-cores.sh -s tests/cypress/integration/check-block-pattern-exists.test.js
  • for WP 5.5 and 5.6 using "name only" command call from your example in the command declaration
  • for WP 5.7+ using extended attribute with block category name
const args = {
  title: testCase.title,
};

if (compare(Cypress.env('WORDPRESS_CORE').toString(), '5.7', '>=')) {
  args.categoryValue = testCase.cat;
}
WordPress core Test
5.2 ⏭ Skip
5.3 ⏭ Skip
5.4 ⏭ Skip
5.5 ❌ Fail
5.6 ❌ Fail
5.7 ✅ Pass
5.8 ✅ Pass
5.9 ✅ Pass
6.0 ❌ Fail
6.1 ❌ Fail

In WP 5.5 and 5.6 Block patterns were in very early stage, I believe we could skip supporting this command for very old versions.

But support of WP 6.0+ is important. Could it be related to __experimentalBlockPatterns?

const allRegisteredPatterns = getSettings().__experimentalBlockPatterns;

@Sidsector9
Copy link
Member

Sidsector9 commented Jun 6, 2022

@cadic I fixed the tests for WP 6.0+. I think 6.0 takes time or maybe defers loading of block patterns.

@faisal-alvi I've made a minor change. I have moved cy.visit() and cy.closeWelcomeGuide() functions outside of checkBlockPatternExists as this function should only deal with identifying patterns. Navigation and other aspects should be handled by the test.

Another reason is that if we use checkBlockPatternExists multiple times in the same test for a post, it will navigate to a new post.

@faisal-alvi
Copy link
Member Author

@Sidsector9 makes sense. However, is it fine to use cy.wait(1000);? Isn't there any alternate? End users might not know to add a wait function before using checkBlockPatternExists.

@Sidsector9 Sidsector9 force-pushed the command/40 branch 2 times, most recently from 2be2101 to a535b32 Compare June 7, 2022 13:15
const shouldIt = testCase.expected ? 'should' : 'should not';
it(`Pattern "${testCase.title}" ${shouldIt} exist in category "${testCase.cat}"`, () => {
// Wait for patterns to load on the post edit page.
cy.wait(1000);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Sidsector9 I wonder why do we need this? Tests run successfully without this wait on my local environment. Maybe there is GitHub-related issue?

Copy link
Member

Choose a reason for hiding this comment

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

@cadic same here. Works well locally but fails on Actions. That is what fixed the issue with WP6.0

@faisal-alvi
Copy link
Member Author

@cadic @Sidsector9 any further updates here? Eager to see this merged!

@cadic
Copy link
Contributor

cadic commented Aug 17, 2022

Failed test is related to another command createTerm, please ignore

@Sidsector9
Copy link
Member

@faisal-alvi @cadic I have removed the wait command, can you check?

@dkotter dkotter removed the request for review from dinhtungdu August 23, 2022 22:15
@faisal-alvi
Copy link
Member Author

@Sidsector9 @cadic I've tried to test this PR, the Cypress is throwing this error:

image

@Sidsector9
Copy link
Member

@faisal-alvi that's expected to fail on local as we're only setting the environment variables when the actions are run.
If you want to run locally then you'll have to define it manually, such as:

CYPRESS_WORDPRESS_CORE=5.8 npm run cypress:run

& Merge branch 'trunk' into command/40
@faisal-alvi
Copy link
Member Author

I have merged the trunk here and it seems like the E2E Cypress tests failing for the other 3 tests that are not related to this PR. The one that is related to this PR, check-block-pattern-exists.test.js ran successfully. I would recommend that we go ahead and merge this one. Please share your thoughts @Sidsector9 @cadic

image

@faisal-alvi faisal-alvi requested a review from cadic November 16, 2022 10:32
Copy link
Contributor

@cadic cadic left a comment

Choose a reason for hiding this comment

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

Thank you @faisal-alvi, this has been waiting for too long, finally LGTM 🔥

@cadic cadic merged commit 85450d9 into trunk Nov 16, 2022
@cadic cadic deleted the command/40 branch November 16, 2022 11:03
github-actions bot pushed a commit that referenced this pull request Nov 16, 2022
…62)

* Command/40 Added `checkBlockPatternExists` command.

* add examples in the documentaion

* try to fix the lower version issue

* using `wrap` to return & skipped pattern insertion

* added 10s waiting as a workaround of slow redner by wp

* suggestion implemented

* declare global var

* command/40 replacing the alerts with simple comments

* Tests for checkBlockPatterExists

* Remove debug output

* Use correct values

* Make test optional for old WordPress core

* Fail-fast for matrix

* replace semver with compare-versions for optional testing

* Add 5.9 to versions list in run-all-cores

* Threshold 5.7

* run-all-cores.sh

* fix: WP 6.0 tests

* try without wait

* try checking with setInterval

Co-authored-by: Faisal Alvi <[email protected]>
Co-authored-by: Max Lyuchin <[email protected]>
Co-authored-by: Siddharth Thevaril <[email protected]>
@jeffpaul jeffpaul modified the milestones: Future Release, 0.1.0 Mar 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

checkBlockPatternExists Migrate custom commands from 10up/publisher-media-kit
5 participants