The `playwright` CI job is also checking code style, but a failure is easily missed

This is very disruptive.

For the MRs I'm working on this is failing like 90%. Pretty annoying.

I think we should run prettier and eslint on tests, but as a separate job (which might be allowed to fail?)

Please review. Do you agree we should spin-off prettier/eslint from this job?

Posted my thoughts. Let's let @justafish decide.

What's certain: the CI results in HEAD are definitely not clear enough. I understand how @isholgueras ended up believing that playwright was broken in HEAD. It requires a very careful reading, because:

… all successes here!
  ✓  58 [firefox] › tests/src/Playwright/globalElements.spec.ts:52:7 › Global elements › Breadcrumbs (8.8s)
  ✓  59 [webkit] › tests/src/Playwright/globalElements.spec.ts:52:7 › Global elements › Breadcrumbs (9.4s)
  59 passed (1.3m)
ERROR: "test:prettier" exited with 1.

… it's really just that one ERROR: … line, that isn't even in red!

Perhaps the better alternative solution is to run prettier first, and hence refuse to run the tests altogether unless the code style requirements are met. Then at least there isn't an ocean of "yay"s followed by a tiny "nay".

Doing a separate job would also allow reporting the code style checking results in a way that GitLab CI presents them correctly. Quoting the UI eslint CI job:

    - node_modules/.bin/eslint . --format=junit --output-file=$CI_PROJECT_DIR/ui-eslint-junit.xml --max-warnings=0
  artifacts:
    expose_as: junit
    expire_in: 6 mos
    when: always
    name: artifacts-$CI_PIPELINE_ID-$CI_JOB_NAME_SLUG
    paths:
      - ui-eslint-junit.xml
    reports:
      junit: ui-eslint-junit.xml

👆 That's what's missing. And we can't combine that into the playwright CI job, because it already needs to report actual test results; code style results are a different consideration.

Perhaps the better alternative solution is to run prettier first, and hence refuse to run the tests altogether unless the code style requirements are met. Then at least there isn't an ocean of "yay"s followed by a tiny "nay".

It doesn't seem right that the tests shouldn't run due to prettier errors. There's nothing more annoying than to not get test results because of a silly code style error. Having a separate job would solve this.

#10: I know, I agree, which is what I wrote immediately after what you quoted 😉 I was merely acknowleding the infrastructure (and hence ecological) impact.

Thanks for opening and fixing this so quickly!

However, we have already split these jobs up in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

If you're happy with the implementation above I'll close this one

and also for a bit of background, it's because prettier and eslint in those forms (i.e. from the top level package.json) were only running on the Playwright tests as we have yet to reconcile this with the ui folder. However in the MR above it's now running on the new cli workspace too.

I'd still merge this as it fixes the prettier error + HEAD is actually broken while that MR has a larger scope + draft MR + needs reviews.
Let's unblock merging the beta blockers 🙏🏽

#14++

Got @justafish's +1 to go ahead and get HEAD green again, because