Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support doubling readonly classes #5804

Closed
wants to merge 15 commits into from
Closed

Support doubling readonly classes #5804

wants to merge 15 commits into from

Conversation

sebastianbergmann
Copy link
Owner

@sebastianbergmann sebastianbergmann commented Apr 7, 2024 •
edited
Loading

  • Refactor test double runtime to hold state in readonly properties
  • Allow readonly classes to be doubled
  • Investigate failing test for cloning of test doubles
  • Emit error when clone is used on the test double for a readonly class on PHP 8.2
  • Test doubling of readonly classes

@sebastianbergmann sebastianbergmann added type/enhancement A new idea that should be implemented feature/test-doubles Test Stubs and Mock Objects labels Apr 7, 2024
@sebastianbergmann sebastianbergmann mentioned this pull request Apr 7, 2024
Closed
@sebastianbergmann sebastianbergmann changed the title Support doubling of readonly classes Support doubling readonly classes Apr 7, 2024
@sebastianbergmann sebastianbergmann force-pushed the test-doubles-for-readonly-classes branch from 14c74b5 to 5af0410 Compare April 9, 2024 05:46
@codecov Codecov
Copy link

codecov bot commented Apr 9, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.13%. Comparing base (e91fafb) to head (b53b04c).

Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #5804      +/-   ##
============================================
+ Coverage     90.11%   90.13%   +0.01%     
- Complexity     6616     6630      +14     
============================================
  Files           697      697              
  Lines         20008    20026      +18     
============================================
+ Hits          18031    18051      +20     
+ Misses         1977     1975       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sebastianbergmann
Copy link
Owner Author

I was able to implement support for doubling readonly classes.

However, this implementation breaks support for cloning test double objects on PHP 8.2 using clone, regardless of whether they were created based on an extendable class, an extendable readonly class, or an interface. With PHP >= 8.3, cloning test double objects continues to work.

I cannot be sure, but I assume that cloning test double objects is not commonly done and that this breakage is acceptable. The current state of this PR reflects this.

Alternatively, we could delay support for doubling readonly classes until PHPUnit 12 which will require PHP 8.3. This way, PHPUnit 11 would not break existing behaviour for PHP 8.2. Of course, it would delay support for doubling readonly classes even more.

@sebastianbergmann sebastianbergmann marked this pull request as ready for review April 9, 2024 05:57
@sebastianbergmann sebastianbergmann force-pushed the test-doubles-for-readonly-classes branch from 5af0410 to d660f26 Compare April 10, 2024 16:04
@sebastianbergmann
Copy link
Owner Author

I may have found a solution to the problem explained in #5804 (comment).

With the current state of this PR, using clone on test doubles for classes that are not declared readonly continues to work with PHP 8.2.

Using clone on test doubles for classes that are declared readonly will not work with PHP 8.2, but this is not a regression as doubling classes that are declared readonly was not supported before.

Using clone on test doubles for classes that are declared readonly works on PHP >= 8.3.

@sebastianbergmann sebastianbergmann added this to the PHPUnit 11.2 milestone Apr 10, 2024
@sebastianbergmann sebastianbergmann deleted the test-doubles-for-readonly-classes branch April 11, 2024 09:03
@sebastianbergmann sebastianbergmann mentioned this pull request Apr 11, 2024
Closed
@sebastianbergmann sebastianbergmann mentioned this pull request Jun 9, 2024
Closed
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Jun 9, 2024
@sbuerk
[TASK] Add phpunit 11.2.0 as conflict version due to regression
6d4daf0 
PHPUnit refactored the double creation to support doubling
readonly classes [1], thus breaking classes where the class
constructor is called with constructor arguments, but calling
itself a class method which is mocked using the `onlyMethods()`
setup option. This leads now to some internal state setup issue
and is reported as `regression` [2] providing steps to reproduce
it.

This change adds `phpunit 11.2.0` as conflict to the monorepo
version to avoid using it in nightly tests for now, until a
proper regression fix has been implemented in phpunit.

Used command(s):

  \
    cat \
    <<< $(jq --indent 1 --tab \
      '."conflict" += {"phpunit/phpunit": "11.2.0"}' \
      composer.json) > composer.json \
    && Build/Scripts/runTests.sh -s composer -- update --lock

[1] sebastianbergmann/phpunit#5804
[2] sebastianbergmann/phpunit#5857

Resolves: #104007
Releases: main
Change-Id: Ia384ff98a9ea4e318738a48bd9ff8afcc4faff15
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/84533
Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Reviewed-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com>
Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>
Tested-by: core-ci <typo3@b13.com>
Reviewed-by: Stefan Bürk <stefan@buerk.tech>
Tested-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com>
Tested-by: Stefan Bürk <stefan@buerk.tech>
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Jun 10, 2024
@sbuerk @lolli42
[TASK] Mitigate PHPUnit NameAlreadyInUseException
e2aff12 
PHPUnit changed the way how double creation is handled
internally and now emits an exception if a manually defined
`mock class name` should be registered. In some places
within the unit tests, manual mock class names have been
used, because the names are needed for cross referencing
at a later point. For this, different approaches have
been used, for example hardcoded MD5 hashes as strings
or creating hashes of static values like `md5('1')`,
and also reused hashes in different places or in
tests using data providers. This now leads to an
exception:

    PHPUnit\Framework\MockObject\Generator\NameAlreadyInUseException

    The name "b70551b2b2db62b6b15a9bbfcbd50614" is already in use

This change mitigates the issue by using
`StringUtility::getUniqueId('somePrefix')` as is already done
in other places.

As a sideeffect, four phpstan baseline entries can be removed.

Used command(s):

  Build/Scripts/runTests.sh -s phpstanGenerateBaseline

[1] sebastianbergmann/phpunit#5804

Resolves: #104005
Releases: main, 12.4
Change-Id: Icc558844275c9ae9f67a0e7c20daa318f5ad6b41
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/84530
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Andreas Wolf <andreas.wolf@typo3.org>
Reviewed-by: Andreas Wolf <andreas.wolf@typo3.org>
reviewtypo3org pushed a commit to TYPO3/typo3 that referenced this pull request Jun 10, 2024
@sbuerk @lolli42
[TASK] Mitigate PHPUnit NameAlreadyInUseException
6b067a2 
PHPUnit changed the way how double creation is handled
internally and now emits an exception if a manually defined
`mock class name` should be registered. In some places
within the unit tests, manual mock class names have been
used, because the names are needed for cross referencing
at a later point. For this, different approaches have
been used, for example hardcoded MD5 hashes as strings
or creating hashes of static values like `md5('1')`,
and also reused hashes in different places or in
tests using data providers. This now leads to an
exception:

    PHPUnit\Framework\MockObject\Generator\NameAlreadyInUseException

    The name "b70551b2b2db62b6b15a9bbfcbd50614" is already in use

This change mitigates the issue by using
`StringUtility::getUniqueId('somePrefix')` as is already done
in other places.

As a sideeffect, four phpstan baseline entries can be removed.

Used command(s):

  Build/Scripts/runTests.sh -s phpstanGenerateBaseline

[1] sebastianbergmann/phpunit#5804

Resolves: #104005
Releases: main, 12.4
Change-Id: Icc558844275c9ae9f67a0e7c20daa318f5ad6b41
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/84531
Tested-by: core-ci <typo3@b13.com>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this pull request Jun 10, 2024
@sbuerk @lolli42
[TASK] Mitigate PHPUnit NameAlreadyInUseException
18c247c 
PHPUnit changed the way how double creation is handled
internally and now emits an exception if a manually defined
`mock class name` should be registered. In some places
within the unit tests, manual mock class names have been
used, because the names are needed for cross referencing
at a later point. For this, different approaches have
been used, for example hardcoded MD5 hashes as strings
or creating hashes of static values like `md5('1')`,
and also reused hashes in different places or in
tests using data providers. This now leads to an
exception:

    PHPUnit\Framework\MockObject\Generator\NameAlreadyInUseException

    The name "b70551b2b2db62b6b15a9bbfcbd50614" is already in use

This change mitigates the issue by using
`StringUtility::getUniqueId('somePrefix')` as is already done
in other places.

As a sideeffect, four phpstan baseline entries can be removed.

Used command(s):

  Build/Scripts/runTests.sh -s phpstanGenerateBaseline

[1] sebastianbergmann/phpunit#5804

Resolves: #104005
Releases: main, 12.4
Change-Id: Icc558844275c9ae9f67a0e7c20daa318f5ad6b41
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/84530
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Andreas Wolf <andreas.wolf@typo3.org>
Reviewed-by: Andreas Wolf <andreas.wolf@typo3.org>
TYPO3IncTeam pushed a commit to TYPO3-CMS/core that referenced this pull request Jun 10, 2024
@sbuerk @lolli42
[TASK] Mitigate PHPUnit NameAlreadyInUseException
55da5d0 
PHPUnit changed the way how double creation is handled
internally and now emits an exception if a manually defined
`mock class name` should be registered. In some places
within the unit tests, manual mock class names have been
used, because the names are needed for cross referencing
at a later point. For this, different approaches have
been used, for example hardcoded MD5 hashes as strings
or creating hashes of static values like `md5('1')`,
and also reused hashes in different places or in
tests using data providers. This now leads to an
exception:

    PHPUnit\Framework\MockObject\Generator\NameAlreadyInUseException

    The name "b70551b2b2db62b6b15a9bbfcbd50614" is already in use

This change mitigates the issue by using
`StringUtility::getUniqueId('somePrefix')` as is already done
in other places.

As a sideeffect, four phpstan baseline entries can be removed.

Used command(s):

  Build/Scripts/runTests.sh -s phpstanGenerateBaseline

[1] sebastianbergmann/phpunit#5804

Resolves: #104005
Releases: main, 12.4
Change-Id: Icc558844275c9ae9f67a0e7c20daa318f5ad6b41
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/84531
Tested-by: core-ci <typo3@b13.com>
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature/test-doubles Test Stubs and Mock Objects type/enhancement A new idea that should be implemented
Projects
None yet
Milestone
PHPUnit 11.2
Development

Successfully merging this pull request may close these issues.
1 participant
@sebastianbergmann