[master] Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6 #34871

phil-davis · 2019-03-22T07:19:44Z

Description

PHPUnit6 reports as "risky" tests that ran the test case but no assert was ever done.

Those are suspicious, because they might never fail.

Often the tests are checking that "something works and does not throw an exception". In that case the test will fail if an exception is thrown (for whatever reason - bug...) In those cases, put an assert of some sort about what the method call in the test is expected to return. If it is a method call that returns something, e.g. returns an int, then check that the return value is an int. If it is a method that does not return anything ("return void") then use assertNull to check that the return value "is not".

And fix other tests that turned out to be "snake oil" on examination (see inline comments).

It seems that the reporting of these sort of tests as "risky" is worth having.

Related Issue

Part of the rabbit-hole being followed from issue #34858

Motivation and Context

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:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

codecov · 2019-03-22T08:12:06Z

Codecov Report

Merging #34871 into master will decrease coverage by 16.95%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #34871       +/-   ##
=============================================
- Coverage     65.35%   48.39%   -16.96%     
=============================================
  Files          1208      109     -1099     
  Lines         69969    10493    -59476     
  Branches       1280     1280               
=============================================
- Hits          45727     5078    -40649     
+ Misses        23870     5043    -18827     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	`53.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`38.17% <ø> (-28.59%)`	`0 <ø> (-18484)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Storage/DAV.php	`59.45% <0%> (-21.64%)`	`0% <0%> (ø)`
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1090 more

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 fa3a310...57b8e0d. Read the comment docs.

codecov · 2019-03-22T08:12:07Z

Codecov Report

Merging #34871 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master   #34871      +/-   ##
============================================
+ Coverage     65.34%   65.35%   +<.01%     
  Complexity    18484    18484              
============================================
  Files          1208     1208              
  Lines         69969    69969              
  Branches       1280     1280              
============================================
+ Hits          45723    45728       +5     
+ Misses        23874    23869       -5     
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	`53.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.76% <ø> (ø)`	`18484 <ø> (ø)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
...eratedfilesharing/lib/Middleware/OcmMiddleware.php	`77.27% <ø> (ø)`	`20 <0> (ø)`	⬇️
lib/private/Files/View.php	`84.6% <0%> (+0.3%)`	`398% <0%> (ø)`	⬇️
lib/private/DB/Connection.php	`66.17% <0%> (+0.73%)`	`49% <0%> (ø)`	⬇️
apps/files_trashbin/lib/Expiration.php	`98.27% <0%> (+1.72%)`	`29% <0%> (ø)`	⬇️

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 51e12cf...d83e81a. Read the comment docs.

phil-davis · 2019-03-22T08:26:15Z

apps/federatedfilesharing/lib/Middleware/OcmMiddleware.php

@@ -93,7 +93,7 @@ public function __construct(
 	 *
 	 * @param string[] $params
 	 *
-	 * @return bool
+	 * @return void


While adjusting OcmMiddlewareTest I saw that the PHPdoc return specified here was wrong.
This method either returns nothing (void) or throws an exception.

phil-davis · 2019-03-22T08:28:05Z

apps/files/tests/Command/ScanTest.php

 			$this->assertContains('Starting scan for user 1 out of 10 (user1)', $output);
 			$this->assertContains('Starting scan for user 1 out of 10 (user11)', $output);
 			$this->assertContains('Starting scan for user 1 out of 10 (user21)', $output);
 			$this->assertContains('Starting scan for user 10 out of 10 (user30)', $output);
+		} else if (\count($groups) === 4) {


This test was doing no assertContains checks for the data set:

[['--groups' => 'group1,group2', '--group' => ['group2','group3']], '']

and so it did not fail, but also was snake oil.

phil-davis · 2019-03-22T08:29:17Z

apps/files/tests/Command/ScanTest.php

+			$this->assertContains('Starting scan for user 11 out of 20 (user21)', $output);
+			$this->assertContains('Starting scan for user 10 out of 10 (user40)', $output);
+		} else {
+			$this->fail(


Make sure that if someone adds test cases with different numbers of groups in the command, that the test will fail until they add appropriate asserts.

phil-davis · 2019-03-22T08:33:37Z

tests/lib/Events/EventEmitterTraitTest.php

@@ -97,6 +97,10 @@ public function testEmittingCallWithAdditionalArgument($className, $eventName, $
 			$this->assertInstanceOf(GenericEvent::class, $calledAfterEvent[1]);
 			$this->assertArrayHasKey('item', $calledAfterEvent[1]);
 			$this->assertEquals('testing', $calledAfterEvent[1]->getArgument('item'));
+		} else {


Previously no checks were happening for the test cases that had an empty after specified. So those test cases were "snake oil". Actually it turns out that the tests are OK - checking with these asserts passes.

phil-davis · 2019-03-22T08:35:00Z

tests/lib/Group/Backend.php

+			$this->backend->createGroup($group),
+			"there was a problem creating $group"
+		);
+		$this->assertFalse(


The first time calling createGroup() returns true (it did it)
The second time returns false - the group already exists.
We can check this stuff, then we will know if that behavior changes.

phil-davis · 2019-03-22T10:26:12Z

Backport stable10 #34876

PVince81 · 2019-03-22T12:51:28Z

@phil-davis can we configure this in PHPUnit so it runs like we want to instead of skipping tests ?

phil-davis · 2019-03-22T13:25:52Z

@phil-davis can we configure this in PHPUnit so it runs like we want to instead of skipping tests ?

@PVince81 I don't understand what you mean? All the tests are actually run by phpunit, it is just that some of them contain no assertSomething() calls, and so phpunit (after running them) reports that they are "risky" because they "cannot fail" in a typical way.

For safety, I put an assertSomething() call in each offending test. That makes sure that the test always asserts something, and thus could fail if the assertion is not true at run-time.

Of course all tests can fail if something that they call throws an exception, but phpunit does not consider only having that to be "good practice". This thread has lots of discussion related to "Testing an exception is not thrown":

sebastianbergmann/phpunit-documentation#171 (comment)

PVince81 · 2019-03-28T13:18:39Z

@phil-davis I was a bit out of context.

I'm surprised that PHPUnit 5 doesn't report such tests as risky already ? Or did we have an option in phpunit.xml to disable this ?

PVince81

👍 fine by me

phil-davis · 2019-03-28T15:15:47Z

@phil-davis I was a bit out of context.

I'm surprised that PHPUnit 5 doesn't report such tests as risky already ? Or did we have an option in phpunit.xml to disable this ?

PHPUnit6 seems to report the risky stuff. The default for beStrictAboutTestsThatDoNotTestAnything was false in PHPUnit5, now the default is true. So we suddenly start seeing these reported in PHPUnit6.

phil-davis added 2 - Developing dev:phpunit-tests labels Mar 22, 2019

phil-davis self-assigned this Mar 22, 2019

phil-davis changed the title ~~Add explicit PHPUnit asserts to unit tests flagged as 'risky'~~ Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6 Mar 22, 2019

phil-davis commented Mar 22, 2019

View reviewed changes

phil-davis added 3 - To Review and removed 2 - Developing labels Mar 22, 2019

phil-davis requested review from DeepDiver1975 and PVince81 March 22, 2019 08:41

phil-davis force-pushed the add-assert-to-risky-unit-tests-phpunit-5 branch from 57b8e0d to 568cb77 Compare March 22, 2019 08:44

phil-davis changed the title ~~Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6~~ [master] Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6 Mar 22, 2019

phil-davis added the backport-request label Mar 22, 2019

This was referenced Mar 22, 2019

Add assert to risky unit tests (with PHPUnit6) #34870

Closed

[stable10] Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6 #34876

Merged

phil-davis removed the backport-request label Mar 22, 2019

phil-davis requested review from VicDeo and jvillafanez March 24, 2019 04:44

phil-davis force-pushed the add-assert-to-risky-unit-tests-phpunit-5 branch from 05d2a6e to d0f643c Compare March 24, 2019 04:44

phil-davis added 3 commits March 25, 2019 10:56

Add explicit PHPUnit asserts to unit tests flagged as 'risky'

d7109f5

Fix ScanTest testMultipleGroups for test input with 4 groups

53a2c19

Adjust IsType for PHPUnit5

d83e81a

phil-davis force-pushed the add-assert-to-risky-unit-tests-phpunit-5 branch from d0f643c to d83e81a Compare March 25, 2019 05:12

PVince81 approved these changes Mar 28, 2019

View reviewed changes

phil-davis merged commit 3f4a433 into master Mar 28, 2019

delete-merged-branch bot deleted the add-assert-to-risky-unit-tests-phpunit-5 branch March 28, 2019 15:16

lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[master] Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6 #34871

[master] Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6 #34871

phil-davis commented Mar 22, 2019 •

edited

Loading

codecov bot commented Mar 22, 2019

codecov bot commented Mar 22, 2019 •

edited

Loading

phil-davis Mar 22, 2019 •

edited

Loading

phil-davis Mar 22, 2019

phil-davis Mar 22, 2019

phil-davis Mar 22, 2019

phil-davis Mar 22, 2019

phil-davis commented Mar 22, 2019

PVince81 commented Mar 22, 2019

phil-davis commented Mar 22, 2019 •

edited

Loading

PVince81 commented Mar 28, 2019

PVince81 left a comment

phil-davis commented Mar 28, 2019

[master] Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6 #34871

[master] Add explicit PHPUnit asserts to unit tests flagged as 'risky' by PHPUnit6 #34871

Conversation

phil-davis commented Mar 22, 2019 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Open tasks:

codecov bot commented Mar 22, 2019

Codecov Report

codecov bot commented Mar 22, 2019 • edited Loading

Codecov Report

phil-davis Mar 22, 2019 • edited Loading

Choose a reason for hiding this comment

phil-davis Mar 22, 2019

Choose a reason for hiding this comment

phil-davis Mar 22, 2019

Choose a reason for hiding this comment

phil-davis Mar 22, 2019

Choose a reason for hiding this comment

phil-davis Mar 22, 2019

Choose a reason for hiding this comment

phil-davis commented Mar 22, 2019

PVince81 commented Mar 22, 2019

phil-davis commented Mar 22, 2019 • edited Loading

PVince81 commented Mar 28, 2019

PVince81 left a comment

Choose a reason for hiding this comment

phil-davis commented Mar 28, 2019

phil-davis commented Mar 22, 2019 •

edited

Loading

codecov bot commented Mar 22, 2019 •

edited

Loading

phil-davis Mar 22, 2019 •

edited

Loading

phil-davis commented Mar 22, 2019 •

edited

Loading