Skip to content

[Tests-Only] Adjust unit tests for the future#37169

Merged
phil-davis merged 4 commits into
masterfrom
adjust-unit-tests-for-the-future
Apr 7, 2020
Merged

[Tests-Only] Adjust unit tests for the future#37169
phil-davis merged 4 commits into
masterfrom
adjust-unit-tests-for-the-future

Conversation

@phil-davis

@phil-davis phil-davis commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

Description

These are some changes that can be made now to PHP unit tests.
phpunit 8 reports various deprecations. See example PR #37168

  1. Remove undeclared property PHPUnit\Framework\Error\Deprecated - not valid/needed any more with phpunit8 - we will also be fine removing this now for phpunit7, if people write new unit test code it will be useful to have the new unit test code fail if it uses things that have been deprecated,

  2. Stop using assertAttributeEquals in tests/lib/ConfigTest.php - that assert is deprecated and being removed. I wrote a function that does an assertEquals for each item.

  3. Adjust createMock calls to remove deprecated array of interface names - createMock() can only be passed a single class/interface. phpunit8 reports:

Passing an array of interface names to createMock() for creating a test double that implements multiple interfaces is deprecated and will no longer be supported in PHPUnit 9.
Passing an array of interface names to getMockBuilder() for creating a test double that implements multiple interfaces is deprecated and will no longer be supported in PHPUnit 9.

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@codecov

codecov Bot commented Mar 26, 2020

Copy link
Copy Markdown

Codecov Report

Merging #37169 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37169   +/-   ##
=========================================
  Coverage     64.87%   64.87%           
  Complexity    19146    19146           
=========================================
  Files          1269     1269           
  Lines         74947    74947           
  Branches       1331     1331           
=========================================
  Hits          48625    48625           
  Misses        25930    25930           
  Partials        392      392           
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 66.07% <ø> (ø) 19146.00 <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8ae99c...d16a43b. Read the comment docs.

@phil-davis phil-davis marked this pull request as ready for review March 26, 2020 10:01
Comment thread tests/lib/ConfigTest.php Outdated
Comment thread tests/lib/ConfigTest.php Outdated
@jvillafanez

Copy link
Copy Markdown
Member

I don't really like to have dummy interfaces to "join" different classes or interfaces even if it's just for tests. From my point of view, there is either a problem in the code or in the tests.
I'm not sure if there are better options.

@phil-davis

phil-davis commented Mar 27, 2020

Copy link
Copy Markdown
Contributor Author

I don't really like to have dummy interfaces to "join" different classes or interfaces even if it's just for tests. From my point of view, there is either a problem in the code or in the tests.
I'm not sure if there are better options.

I agree. Where I could, I found a class or interface that already implemented at least the multiple interfaces. It seemed odd to me that existing tests mock a combined set of interfaces, but there is nowhere in the actual codebase that seems to use exactly that combined set of interfaces.

I didn't want to mess with actual "real" interface/class hierarchy because that would add potential for getting it wrong, and refactoring (redesigning?) a lot of that stuff seemed out-of-scope.

Some related discussion that gave me the idea: https://stackoverflow.com/questions/8544171/phpunit-stubbing-multiple-interfaces

@phil-davis phil-davis mentioned this pull request Mar 28, 2020
@phil-davis phil-davis force-pushed the adjust-unit-tests-for-the-future branch from 633bd80 to 03366f3 Compare April 7, 2020 03:45
@phil-davis phil-davis requested a review from jvillafanez April 7, 2020 03:55

@jvillafanez jvillafanez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd write a fixme or todo comment for each of the dummy interfaces, mainly to ensure we'll know in the future that we didn't have a better choice.

@phil-davis

Copy link
Copy Markdown
Contributor Author

ToDo comments have been added.

@phil-davis phil-davis merged commit 6504861 into master Apr 7, 2020
@delete-merged-branch delete-merged-branch Bot deleted the adjust-unit-tests-for-the-future branch April 7, 2020 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants