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 passed

In 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

Command icon 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:

Comments

xjm created an issue. See original summary.

xjm’s picture

Issue summary: View changes

xjm’s picture

Status: Active » Needs review
xjm’s picture

Hm, still a bit confusiong:

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
[11:57:07] '/var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css' is being checked.
[11:57:07] '/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.
File core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css does not match the compiled CSS asset.
STYLELINT: core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css passed

The "passed" message is technically true -- the file itself is valid -- but it is still confusing.

Pushed a commit to improve it.

xjm’s picture

OK, this seems clearer:

----------------------------------------------------------------------------------------------------
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
[13:26:26] '/var/www/html/core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css' is being checked.
[13:26: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: File core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css does not match the compiled CSS asset.

STYLELINT: core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css passed

xjm’s picture

Issue summary: View changes
xjm’s picture

Issue summary: View changes
spokje’s picture

Looks like a nice improvement to me.

If I had to nitpick (which I now gonna do):

does match its CSS file. Try recompiling the CSS.

Do we want to be even more explicit and mention the yarn command that is needed to do the recompile?

xjm’s picture

Issue summary: View changes

Good suggestion. Nice and clear:

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
spokje’s picture

Like it!

But just now noticed (sorry) that we're apparently missing a "not":

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

should (probably?) be:

ERROR: The compiled CSS from
       core/modules/announcements_feed/css/announcements_feed.toolbar.pcss.css
       does not match its CSS file. Recompile the CSS with:
       yarn run build:css
spokje’s picture

Status: Needs review » Reviewed & tested by the community

This is why we don't commit our own patches.

This 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.

alexpott’s picture

Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 081f569052 to 10.1.x and ed4038601c to 10.0.x and 8e78c9376d to 9.5.x. Thanks!

Helpful messages++

  • alexpott committed 081f5690 on 10.1.x
    Issue #3331818 by xjm, Spokje: commit-code-check.sh gives unclear output...

  • alexpott committed ed403860 on 10.0.x
    Issue #3331818 by xjm, Spokje: commit-code-check.sh gives unclear output...

  • alexpott committed 8e78c937 on 9.5.x
    Issue #3331818 by xjm, Spokje: commit-code-check.sh gives unclear output...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.