Problem/Motivation
Discovered in #3206643-135: Project messaging channel in core (as experimental). When there was a change to the CSS but it was not recompiled, the code quality checks failed without explicitly saying why. The testbot output was just:
----------------------------------------------------------------------------------------------------
Checking core/modules/announcements_feed/css/announcements_feed.toolbar.css
core/modules/announcements_feed/css/announcements_feed.toolbar.css passed
----------------------------------------------------------------------------------------------------
Checking core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css
yarn run v1.22.19
$ node ./scripts/css/postcss-build.js --check --file /var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css
[01:52:51] '/var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css' is being checked.
[01:52:52] '/var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css' is not updated.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
STYLELINT: core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css passedIn the verbose output from the testrunner, it said:
16:42:56 ----------------------------------------------------------------------------------------------------
16:42:56 Checking core/modules/announcements_feed/css/announcements_feed.toolbar.css
16:42:56
16:42:56 core/modules/announcements_feed/css/announcements_feed.toolbar.css passed
16:42:56
16:42:56 ----------------------------------------------------------------------------------------------------
16:42:56 Checking core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css
16:42:56
16:42:56 yarn run v1.22.19
16:42:56 $ node ./scripts/css/postcss-build.js --check --file /var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css
16:42:57 [22:42:57] '/var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css' is being checked.
16:42:58 [22:42:58] '/var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css' is not updated.
16:42:58 error Command failed with exit code 1.
16:42:58 info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
16:42:58 STYLELINT: core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css passed
16:42:58
16:42:58 ---------------------------------------------------------------------------------------------------...with no indication of what the error was.
I think the problem is around here:
# PostCSS
if [[ "$COMPILE_CHECK" == "1" ]] && [[ -f "$TOP_LEVEL/$BASENAME.pcss.css" ]]; then
cd "$TOP_LEVEL/core"
yarn run build:css --check --file "$TOP_LEVEL/$BASENAME.pcss.css"
CORRECTCSS=$?
if [ "$CORRECTCSS" -ne "0" ]; then
# No need to write any output the yarn run command will do this for
# us.
STATUS=1
fi
cd $TOP_LEVEL
fi
fi
There is no code to print a message if $STATUS is 1. The inline comment indicates an overconfidence in the usefulness of the debug output from Yarn.
Proposed resolution
Change the script to output s clearer message, e.g.:
Checking core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css
yarn run v1.22.19
$ node ./scripts/css/postcss-build.js --check --file /var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css
[21:22:26] '/var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css' is being checked.
[21:22:27] '/var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css' is not updated.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
ERROR: The compiled CSS from
core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css
does match its CSS file. Recompile the CSS with:
yarn run build:css
STYLELINT: core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css passed
Remaining tasks
TBD
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Issue fork drupal-3331818
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3331818-pcss-check
changes, plain diff MR !3216
- 3331818-FAIL
changes, plain diff MR !3217
Comments
Comment #2
xjmComment #5
xjmComment #6
xjmHm, still a bit confusiong:
The "passed" message is technically true -- the file itself is valid -- but it is still confusing.
Pushed a commit to improve it.
Comment #7
xjmOK, this seems clearer:
Comment #8
xjmComment #9
xjmComment #10
spokjeLooks like a nice improvement to me.
If I had to nitpick (which I now gonna do):
Do we want to be even more explicit and mention the yarn command that is needed to do the recompile?
Comment #11
xjmGood suggestion. Nice and clear:
Comment #12
spokjeLike it!
But just now noticed (sorry) that we're apparently missing a "not":
should (probably?) be:
Comment #13
spokjeThis is why I review my reviews...
Anyway:
- This looks like a very nice explanatory addition to an obscure error message.
- Test only MR fails
- Real MR passes (trying to look into my crystal ball here)
- Code changes are sensible.
If TestBot indeed returns green on the real MR, this is RTBC for me.
Comment #14
alexpottCommitted and pushed 081f569052 to 10.1.x and ed4038601c to 10.0.x and 8e78c9376d to 9.5.x. Thanks!
Helpful messages++