-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 workflows: parity with Core #48603
Conversation
Flaky tests detected in 599bfb6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4297013819
|
While working on porting changes from core back to Gutenberg at #48624 I realized there are some lint checks that differ in the two codebases. Searched for issues/PR and this came up. I wanted to leave a note for using that class as a test case, in case that helps. |
Thanks for flagging this issue. I'm not sure if it's due to linter checks because currently, both repositories use almost the same set of linter rules. However, I will check it. Thank you! |
Ah, I realize now that I should have provided better examples, sorry. These are the two I ran into:
What I've done to test is: copy the class from core into the gutenberg file. Run the Hope this helps! |
Thanks for elaborating on your initial comment, @oandregal. Even though There are Trac issues discussing the enabling of |
c7ec651
to
ee8c64d
Compare
Before starting the code review, make sure that you have read the |
f753ea0
to
91f0606
Compare
91f0606
to
0d91212
Compare
2. Move the PHPUnit CI job from unit-test.yml to phpunit-tests.yml. 3. Move the JavaScript Tests from unit-test.yml to javascript-tests.yml. 4. Rename the Static Analysis (Linting, License, Type checks...) workflow to Coding Standards and move it to coding-standards.yml. 5. Move the PHP coding standards job from unit-tests.yml to coding-standards.yml. 6. Rename the All job to JavaScript coding standards and move it to coding-standards.yml. 7. Rename the JavaScript job to Jest Tests. 8. Change the composer versions of squizlabs/php_codesniffer, phpcompatibility/phpcompatibility-wp, and wp-coding-standards/wpcs to use the same package versions as in Core.
0d91212
to
6adbb3e
Compare
@@ -3,9 +3,6 @@ | |||
<description>Sniffs for WordPress plugins, with minor modifications for Gutenberg</description> | |||
|
|||
<config name="testVersion" value="5.6-"/> | |||
<rule ref="PHPCompatibilityWP"> | |||
<include-pattern>*\.php$</include-pattern> | |||
</rule> | |||
|
|||
<rule ref="WordPress-Core"/> | |||
<rule ref="WordPress-Docs"/> |
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.
Core doesn't use the WordPress-Docs
ruleset. For better parity, may want to consider removing this ruleset or at a minimum discussing why it's needed.
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.
Core doesn't use it (yet) as the number of violations thrown was huge last time we checked, so a clean up is needed first.
I wouldn't consider that a reason not to use the ruleset here though. Better for new code to go in clean, don't you agree ?
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.
I wouldn't consider that a reason not to use the ruleset here though. Better for new code to go in clean, don't you agree ?
Consider this scenario:
Code is changed in Core that then needs to be brought back to Gutenberg.
This happens a lot during release cycles. With Core not using this same ruleset, it can create scenarios where the code being brought back to Gutenberg does not pass the sniffs. When this happens, it can cause extra work to keep the code synchronized between Core and Gutenberg.
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.
The Docs ruleset doesn't examine code, only documentation.
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.
Hey @jrfnl,
Can you take a look at these comments, which are the source of why I first raised the question:
#48603 (comment)
#48603 (comment)
What do you think?
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.
In both cases mentioned here, the docs in the GB repo are better and the docs in WP Core should be updated.
As Core isn't (yet) using the Docs ruleset, there is no reason why those updates should conflict with existing rules in WP Core.
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.
@jrfnl Yes, but WordPress-Docs
causes linter errors when code gets backported from Core to Gutenberg, making it harder to keep both projects in sync (as the comment above showed).
Based on my understanding, WordPress-Docs
could be temporarily disabled to make the backporting process simpler, and then re-enabled when Core supports it.
A decision needs to be made regarding whether the simplicity of backporting code takes precedence over enforcing the WordPress-Docs
standard.
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.
In both cases mentioned #48603 (comment), the docs in the GB repo are better and the docs in WP Core should be updated.
The docs may be better, but with this linting ruleset, they do not have parity with Core.
But without the ruleset in Core, how will contributors know Core's changes comply before porting the code back to Gutenberg? They won't. Thus it will cause more work and potentially further cause the code to be out-of-sync.
The goal of this PR is to achieve Core parity. Once Core introduces this ruleset, then Gutenberg can once again add it back into its workflow. But until then, this ruleset means Gutenberg and Core do not have parity and thus, more work is required as noted when porting Core changes to Gutenberg.
IMO this ruleset needs to be removed from Gutenberg to achieve Core parity.
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.
Also noting, this ruleset needs to first be adopted in Core. Then it can be (re)adopted in Gutenberg.
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.
I'm closing this PR in favor of #56487, with the intention of continuing the discussion on coding standards, specifically the |
Please, don't merge this PR yet, as some Github actions have to be set as required before merging this PR.
What?
This PR aims to:
Why?
How?
Unit Tests
workflow and theunit-test.yml
file.unit-test.yml
tophpunit-tests.yml
.unit-test.yml
tojavascript-tests.yml
.Static Analysis (Linting, License, Type checks...)
workflow toCoding Standards
and moves it tocoding-standards.yml
.PHP coding standards
job fromunit-tests.yml
tocoding-standards.yml
.All
job toJavaScript coding standards
and move it tocoding-standards.yml
.JavaScript
job toJest Tests
.squizlabs/php_codesniffer,
phpcompatibility/phpcompatibility-wp, and
wp-coding-standards/wpcs` packages now use the same package versions as in Core.Testing Instructions
Waiting for status to be reported
messages as these jobs were removed and other jobs were created to replace them.Please, don't merge this PR yet, as some Github actions have to be set as required before merging this PR.
Testing Instructions for Keyboard
Screenshots or screencast