Problem/Motivation

phpstan and phpunit tests are failing in ci because of an invalid reference to quickedit and a call to a class that only exists in diff v2 when we allow support for diff v1.

Also, this core change to the HTML broke the phpunit test: https://git.drupalcode.org/project/drupal/-/commit/faec1421c65a694b320f6...

I propose tolerating extra classes on the selector just in case this happens again.

Issue fork json_field-3620856

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:

About issue forks

Comments

ptmkenny created an issue. See original summary.

ptmkenny’s picture

ptmkenny commented
Title: phpstan and phpunit are broken due to quickedit » phpstan and phpunit are broken due to quickedit and diff v1

ptmkenny opened merge request !62

ptmkenny’s picture

ptmkenny commented
Issue summary: View changes
ptmkenny’s picture

ptmkenny commented
Title: phpstan and phpunit are broken due to quickedit and diff v1 » phpstan and phpunit are broken due to quickedit reference, support for diff v1, and 11.3 core HTML change
Issue summary: View changes
ptmkenny’s picture

ptmkenny commented
Status: Active » Needs review

All tests now pass. This is ready to go so that we can check whether other MRs pass the tests.

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented

I've checked out this branch in my local DDEV container and can confirm the same behaviour as pipeline #947668:

PHPCS

There are no PHPCS coding standards errors or warnings

PHPStan

Note: Using configuration file web/modules/custom/json_field/phpstan.neon.
[OK] No errors

PHPUnit

OK, but there were issues!
Tests: 19, Assertions: 97, Deprecations: 1, PHPUnit Deprecations: 61

+1 RTBC

ptmkenny’s picture

ptmkenny commented

@kieran.cott Thank you for testing, but why are you testing against a ddev container? We already have automated tests to check the pipeline results.

If you are going to test against ddev, please share the details of your setup (core version, PHP version, database version, etc.)

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented

I thought it would be more thorough testing than just verifying that I have clicked through to pipeline #947668 and observed it as green - though feel free to disregard comment #7, and this comment, if that is indeed the limit of the testing scope.

I have configured my DDEV (1.25.2) container to match the CI pipeline as defined in the module's .gitlab-ci.yml (i.e. Drupal GitLab Templates 1.16.0, and the Kubernetes runner with GitLab Runner 19.3.1) wherever it can be, that is:

Drupal - 11.4.5 (exact match)
PHP - 8.3.33 (exact match)
PHPUnit - 11.5.56 (exact match)
PHPStan - 2.2.13 (exact match)
PHPCS - 3.13.6 (exact match)
Drupal Coder - 8.3.31 (exact match)
Composer - 2.10.2 (exact match)
Database - MariaDB 11.8.6 (substitute for CI pipeline's MySQL 8 image)
OS/container - Debian 13, nginx-fpm (substitute for CI pipeline's Ubuntu Apache image)
Architecture - Docker Desktop (substitute for CI pipeline's Linux amd64)

ptmkenny’s picture

ptmkenny commented

@kieran.cott Thanks for the additional information.

If you are going to test in ddev, I think it may be useful to configure your container to be a little (or a lot) different than what is used in the CI tests. This is because with the same configuration, you should almost certainly get the same result as the CI pipeline. But the module supports multiple versions of PHP, core, and so on, all of which can affect test results. So running the tests in more diverse environments can catch test failures that occur, say, on PHP 8.3 but not PHP 8.5.

That said, .gitlab-ci.yml can also be configured to run such tests (for example, this module has next_major and previous_minor available; you can open the pipeline and click the play button to start them-- although don't do that for next major here, because it will fail), and if you run the additional tests in the issue pipeline, everyone can easily check the results, because they're public.