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

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

Merged
merged 3 commits into from
Mar 28, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7109f5
Add explicit PHPUnit asserts to unit tests flagged as 'risky'
phil-davis Mar 22, 2019
53a2c19
Fix ScanTest testMultipleGroups for test input with 4 groups
phil-davis Mar 22, 2019
d83e81a
Adjust IsType for PHPUnit5
phil-davis Mar 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/federatedfilesharing/lib/Middleware/OcmMiddleware.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public function __construct(
*
* @param string[] $params
*
* @return bool
* @return void
Copy link
Contributor Author

@phil-davis phil-davis Mar 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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

*
* @throws BadRequestException
*/
Expand Down
12 changes: 9 additions & 3 deletions apps/federatedfilesharing/tests/Middleware/OcmMiddlewareTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,9 @@ public function testAssertNotNull($input, $expectedResult) {
if (\is_string($expectedResult)) {
$this->expectException($expectedResult);
}
$this->ocmMiddleware->assertNotNull($input);
$this->assertNull(
$this->ocmMiddleware->assertNotNull($input)
);
}

public function dataTestAssertNotNull() {
Expand Down Expand Up @@ -204,7 +206,9 @@ public function testAssertIncomingSharingEnabled($isSharingAppEnabled, $isIncomi
if ($isExceptionExpected) {
$this->expectException(NotImplementedException::class);
}
$this->ocmMiddleware->assertIncomingSharingEnabled();
$this->assertNull(
$this->ocmMiddleware->assertIncomingSharingEnabled()
);
}

public function dataTestAssertIncomingSharingEnabled() {
Expand All @@ -225,7 +229,9 @@ public function testAssertOutgoingSharingEnabled($isSharingAppEnabled, $isOutgoi
if ($isExceptionExpected) {
$this->expectException(NotImplementedException::class);
}
$this->ocmMiddleware->assertOutgoingSharingEnabled();
$this->assertNull(
$this->ocmMiddleware->assertOutgoingSharingEnabled()
);
}

public function dataTestAssertOutgoingSharingEnabled() {
Expand Down
12 changes: 10 additions & 2 deletions apps/files/tests/Command/ScanTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,20 @@ public function testMultipleGroups($input) {
if (\count($groups) === 2) {
$this->assertContains('Starting scan for user 1 out of 10 (user1)', $output);
$this->assertContains('Starting scan for user 1 out of 10 (user11)', $output);
}
if (\count($groups) === 3) {
} elseif (\count($groups) === 3) {
$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);
} elseif (\count($groups) === 4) {
$this->assertContains('Starting scan for user 1 out of 10 (user1)', $output);
$this->assertContains('Starting scan for user 1 out of 20 (user11)', $output);
$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(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"testMultipleGroups supports testing with 2,3, or 4 groups but the input has $groups groups"
);
}
}

Expand Down
4 changes: 3 additions & 1 deletion apps/files_sharing/tests/AppInfo/ApplicationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
class ApplicationTest extends TestCase {
public function testConstructor() {
$app = new Application();
$app->getContainer()->query('Share20OcsController');
$this->assertNotNull(
$app->getContainer()->query('Share20OcsController')
);
}
}
8 changes: 6 additions & 2 deletions apps/files_trashbin/tests/StorageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,9 +747,13 @@ public function testSingleStorageDeleteFileLoggedOut() {
$this->logout();

if (!$this->userView->file_exists('test.txt')) {
$this->markTestSkipped('Skipping since the current home storage backend requires the user to logged in');
$this->markTestSkipped(
'Skipping since the current home storage backend requires the user to be logged in'
);
} else {
$this->userView->unlink('test.txt');
$this->assertNotNull(
$this->userView->unlink('test.txt')
);
}
}

Expand Down
4 changes: 3 additions & 1 deletion apps/files_versions/tests/FileHelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ public function testCreateMissingDirectories() {
[$this->equalTo(FileHelper::VERSIONS_RELATIVE_PATH . '/some/long/path')],
[$this->equalTo(FileHelper::VERSIONS_RELATIVE_PATH . '/some/long/path/to')]
);
$fileHelper->createMissingDirectories($viewMock, $path);
$this->assertNull(
$fileHelper->createMissingDirectories($viewMock, $path)
);
}

/**
Expand Down
9 changes: 7 additions & 2 deletions tests/lib/DB/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use Doctrine\DBAL\Platforms\SqlitePlatform;
use OC\DB\MDB2SchemaManager;
use OCP\DB\QueryBuilder\IQueryBuilder;
use PHPUnit_Framework_Constraint_IsType as IsType;

/**
* Class Connection
Expand Down Expand Up @@ -181,19 +182,23 @@ public function testSetValuesOverWritePreconditionFailed() {

public function testSetValuesSameNoError() {
$this->makeTestTable();
$this->connection->setValues('table', [
$numNewRows = $this->connection->setValues('table', [
'integerfield' => 1
], [
'textfield' => 'foo',
'clobfield' => 'not_null'
]);

$this->assertInternalType(IsType::TYPE_INT, $numNewRows);

// this will result in 'no affected rows' on certain optimizing DBs
// ensure the PreConditionNotMetException isn't thrown
$this->connection->setValues('table', [
$numNewRows = $this->connection->setValues('table', [
'integerfield' => 1
], [
'textfield' => 'foo'
]);

$this->assertInternalType(IsType::TYPE_INT, $numNewRows);
}
}
4 changes: 4 additions & 0 deletions tests/lib/Events/EventEmitterTraitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

$this->assertEquals($calledAfterEvent[0], "$className.after$eventName");
$this->assertArrayHasKey('item', $calledAfterEvent[1]);
$this->assertEquals('testing', $calledAfterEvent[1]->getArgument('item'));
}
}
}
6 changes: 4 additions & 2 deletions tests/lib/Files/Storage/LocalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
namespace Test\Files\Storage;

use OC\Files\Storage\Local;
use PHPUnit_Framework_Constraint_IsType as IsType;

/**
* Class LocalTest
Expand Down Expand Up @@ -99,7 +100,7 @@ public function testDisallowSymlinksOutsideDatadir() {
$storage->file_put_contents('sym/foo', 'bar');
}

public function testDisallowSymlinksInsideDatadir() {
public function testAllowSymlinksInsideDatadir() {
$subDir1 = $this->tmpDir . 'sub1';
$subDir2 = $this->tmpDir . 'sub1/sub2';
$sym = $this->tmpDir . 'sub1/sym';
Expand All @@ -110,7 +111,8 @@ public function testDisallowSymlinksInsideDatadir() {

$storage = new \OC\Files\Storage\Local(['datadir' => $subDir1]);

$storage->file_put_contents('sym/foo', 'bar');
$numBytes = $storage->file_put_contents('sym/foo', 'bar');
$this->assertInternalType(IsType::TYPE_INT, $numBytes);
}

/**
Expand Down
10 changes: 8 additions & 2 deletions tests/lib/Group/Backend.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,13 @@ public function testSearchUsers() {
public function testAddDouble() {
$group = $this->getGroupName();

$this->backend->createGroup($group);
$this->backend->createGroup($group);
$this->assertTrue(
$this->backend->createGroup($group),
"there was a problem creating $group"
);
$this->assertFalse(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

$this->backend->createGroup($group),
"there was a problem creating $group a second time"
);
}
}
2 changes: 1 addition & 1 deletion tests/lib/Repair/RepairMismatchFileCachePathTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,6 @@ public function testVersions($version) {
->method('info')
->with($this->repair->getName() . ' is not executed');
}
$this->repair->run($outputMock);
$this->assertNull($this->repair->run($outputMock));
}
}
6 changes: 5 additions & 1 deletion tests/lib/Security/CSP/ContentSecurityPolicyManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ public function setUp() {
}

public function testAddDefaultPolicy() {
$this->contentSecurityPolicyManager->addDefaultPolicy(new \OCP\AppFramework\Http\ContentSecurityPolicy());
$this->assertNull(
$this->contentSecurityPolicyManager->addDefaultPolicy(
new \OCP\AppFramework\Http\ContentSecurityPolicy()
)
);
}

public function testGetDefaultPolicyWithPolicies() {
Expand Down
36 changes: 27 additions & 9 deletions tests/lib/Share20/ManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1202,7 +1202,9 @@ public function testUserCreateChecksShareWithGroupMembersOnlySharedGroup() {
->with($path)
->willReturn([]);

$this->invokePrivate($this->manager, 'userCreateChecks', [$share]);
$this->assertNull(
$this->invokePrivate($this->manager, 'userCreateChecks', [$share])
);
}

/**
Expand Down Expand Up @@ -1337,7 +1339,9 @@ public function testUserCreateChecksIdenticalPathNotSharedWithUser() {
->with($path)
->willReturn([$share2]);

$this->invokePrivate($this->manager, 'userCreateChecks', [$share]);
$this->assertNull(
$this->invokePrivate($this->manager, 'userCreateChecks', [$share])
);
}

/**
Expand Down Expand Up @@ -1431,7 +1435,9 @@ public function testGroupCreateChecksShareWithGroupMembersOnlyInGroup() {
['core', 'shareapi_allow_group_sharing', 'yes', 'yes'],
]));

$this->invokePrivate($this->manager, 'groupCreateChecks', [$share]);
$this->assertNull(
$this->invokePrivate($this->manager, 'groupCreateChecks', [$share])
);
}

/**
Expand Down Expand Up @@ -1486,7 +1492,9 @@ public function testGroupCreateChecksPathAlreadySharedWithDifferentGroup() {
['core', 'shareapi_allow_group_sharing', 'yes', 'yes'],
]));

$this->invokePrivate($this->manager, 'groupCreateChecks', [$share]);
$this->assertNull(
$this->invokePrivate($this->manager, 'groupCreateChecks', [$share])
);
}

/**
Expand Down Expand Up @@ -1554,7 +1562,9 @@ public function testLinkCreateChecksPublicUpload() {
['core', 'shareapi_allow_public_upload', 'yes', 'yes']
]));

$this->invokePrivate($this->manager, 'linkCreateChecks', [$share]);
$this->assertNull(
$this->invokePrivate($this->manager, 'linkCreateChecks', [$share])
);
}

public function testLinkCreateChecksReadOnly() {
Expand All @@ -1569,7 +1579,9 @@ public function testLinkCreateChecksReadOnly() {
['core', 'shareapi_allow_public_upload', 'yes', 'no']
]));

$this->invokePrivate($this->manager, 'linkCreateChecks', [$share]);
$this->assertNull(
$this->invokePrivate($this->manager, 'linkCreateChecks', [$share])
);
}

/**
Expand Down Expand Up @@ -1601,13 +1613,17 @@ public function testPathCreateChecksContainsNoSharedMount() {

$this->mountManager->method('findIn')->with('path')->willReturn([$mount]);

$this->invokePrivate($this->manager, 'pathCreateChecks', [$path]);
$this->assertNull(
$this->invokePrivate($this->manager, 'pathCreateChecks', [$path])
);
}

public function testPathCreateChecksContainsNoFolder() {
$path = $this->createMock(File::class);

$this->invokePrivate($this->manager, 'pathCreateChecks', [$path]);
$this->assertNull(
$this->invokePrivate($this->manager, 'pathCreateChecks', [$path])
);
}

public function dataIsSharingDisabledForUser() {
Expand Down Expand Up @@ -3427,7 +3443,9 @@ public function testUpdateShareForRecipient() {

$this->defaultProvider->method('move')->with($share, 'recipient')->will($this->returnArgument(0));

$this->manager->updateShareForRecipient($share, 'recipient');
$this->assertNull(
$this->manager->updateShareForRecipient($share, 'recipient')
);
}

public function testGetSharedWith() {
Expand Down