-
Notifications
You must be signed in to change notification settings - Fork 148
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
GitHub Actions Migration #2451
GitHub Actions Migration #2451
Conversation
* Introduce validation-coding-standards.yml * Change branch for testing * Remove branch * Add - * Re-add branch * update * Yarn install before phpcs * Composer install * Composer lint * Refactor composer install * Tweak * Tweak * Test remove composer install * Remove Install Composer steps * Switch action trigger to pull_request * yarn install --immutable * actions/checkout@v3 * Updates
This reverts commit 68668ae.
This reverts commit abc35d8.
This allows us to wait until the data object is ready.
We need to ship this feature, and will revisit the issue with Firefox.
@olafleur while we are still completing the deploy to wp.org portion of this task, please go ahead and start to review, as there are some big changes included. |
|
||
disableGutenbergFeatures(); | ||
goTo( '/wp-admin/post-new.php?post_type=post' ).then( () => { | ||
cy.wait( 2000 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you still execute it in local now? In my tests, the wait was not needed, so that is a 2 seconds which is useless in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I could not get this working in GitHub Actions consistently with Cypress. Seems the code executes too fast, even though it is waiting on the data object. We would possibly add some additional check for the featureActive portion, but will suffice until we can complete the Playwright migration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, no problem on the waiting on the CI (we dit 10s before, so 2s is an improvement). I was more wondering why you removed the if that made the tests not wait in local (as it is not needed).
if: ${{ inputs.theme == 'go' }} | ||
run: | | ||
cd $(wp-env install-path)/go | ||
mkdir -p coblocks/icons |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This custom SVG icon was not Go-specific, but CoBlocks specific (the Icon block), so it should be executed for any theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may need to circle back to this, then. As we will need to find an alternate way to use WP-ENV and add these into any theme within tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but just to give a little context, we added this last year because we realized that when we added a custom icon (like specified here : https://github.com/godaddy-wordpress/coblocks/blob/master/docs/hooks/icons-block.md ) it would break the Icon block. So, the idea is just to make sure that we don't break that functionality again in CoBlocks by simulating the user adding a custom icon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've got this covered in the latest change: 1bed6e1
@@ -2,7 +2,7 @@ const { defineConfig } = require( 'cypress' ); | |||
|
|||
module.exports = defineConfig( { | |||
chromeWebSecurity: false, | |||
defaultCommandTimeout: 120000, | |||
defaultCommandTimeout: 20000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's nice if we can reduce that time and not have flaky tests :)
So, if I understand correctly, we don't test for variations of themes or browsers for the moment, right? |
Correct. We can add variations for themes, pretty quickly. I'll have to spend some time debugging why Firefox is not working in GitHub Actions, but let's tackle that in an alternate task. |
This pull request handles the migration from CircleCI to GitHub Actions.