Acceptance test refactoring and bug fixes #31523
Conversation
phil-davis
changed the title
phil-davis
force-pushed
the
acceptance-test-use-01
branch
from
d45cfea to
029ec58
Compare
phil-davis
force-pushed
the
acceptance-test-use-01
branch
from
029ec58 to
9027a0a
Compare
Codecov Report
@@ Coverage Diff @@
## master #31523 +/- ##
=========================================
Coverage 62.61% 62.61%
Complexity 18273 18273
=========================================
Files 1147 1147
Lines 68627 68627
Branches 1234 1234
=========================================
Hits 42971 42971
Misses 25295 25295
Partials 361 361
Continue to review full report at Codecov.
|
| @@ -185,10 +185,10 @@ Feature: tags | |||
| When user "user0" adds the tag "MyFirstTag" to "/myFileToTag.txt" owned by "user0" using the API | |||
| And user "another_admin" adds the tag "MySecondTag" to "/myFileToTag.txt" shared by "user0" using the API | |||
| Then the HTTP status code should be "201" | |||
| And file "/myFileToTag.txt" shared by "user0" should have the following tags for "another_admin" | |||
There was a problem hiding this comment.
Actually, "user0" was not used by this step. It does not matter here which user is the one who shared the file. So I removed that from the step definition.
| @@ -86,7 +86,7 @@ Feature: trashbin-new-endpoint | |||
| And user "user0" has deleted file "/textfile0.txt" | |||
| And user "user0" has deleted file "/textfile1.txt" | |||
| And as "user0" the file "/textfile0.txt" should exist in trash | |||
| And as "user0" the file "/textfile0.txt" should exist in trash | |||
| And as "user0" the file "/textfile1.txt" should exist in trash | |||
There was a problem hiding this comment.
Dumb cut-paste error was here - the test was being doen twice on the same file.
| @@ -929,12 +929,12 @@ public function statusPhpRespondedShouldMatch(PyStringNode $jsonExpected) { | |||
| $jsonExpectedDecoded['version'] = \trim($version[1]); | |||
| $jsonExpectedDecoded['versionstring'] = \trim($versionString[1]); | |||
| $jsonExpectedEncoded = \json_encode($jsonExpectedDecoded); | |||
| PHPUnit\Framework\Assert::assertEquals( | |||
There was a problem hiding this comment.
Moved this up here so the IDE does not shout at me about the vars being maybe undefined.
| @@ -243,7 +245,6 @@ public function theseUsersHaveBeenCreated($doNotInitialize, TableNode $table) { | |||
| * @return void | |||
| */ | |||
| public function theAppHasBeenDisabled($app) { | |||
| $client = new Client(); | |||
There was a problem hiding this comment.
This was a random unused var.
| @@ -320,7 +321,6 @@ private function checkUserDownload($url, $options, $mimeType) { | |||
| // read everything | |||
| $buf .= $body->read(8192); | |||
| } | |||
| $mimeType = 'text/plain'; | |||
There was a problem hiding this comment.
This was accidentally left hard-coded here when the function was added recently.
Actually $mimeType is passed in as a parameter.
| $elementRows = $TableNode->getRows(); | ||
|
|
||
| if ($elementRows[0][0] === '') { | ||
| //It shouldn't have public shares | ||
| PHPUnit_Framework_Assert::assertEquals(\count($dataResponded), 0); | ||
| return 0; | ||
| return; |
There was a problem hiding this comment.
We just want to return early in this case - no need to return 0
| @@ -36,7 +36,7 @@ | |||
| */ | |||
| public function emptyTrashbin($user) { | |||
| $body = new \Behat\Gherkin\Node\TableNode( | |||
| [['allfiles', 'true'], ['dir', '%2F']] | |||
There was a problem hiding this comment.
Sending an encoded \ using %2F does not work in the body. When the dir is "hidden" inside the body then there is no need to urlEncode anything.
The test previously passed only because elementIsNotInTrashCheckingOriginalPath() was broken - that Then gherkin step would always pass, even though the trash bin had not actually been emptied.
| ) { | ||
| PHPUnit_Framework_Assert::assertFalse( | ||
| $this->isInTrash($user, $entryText, $originalPath), |
There was a problem hiding this comment.
This was the broken line - isInTrash() actually takes 2 parameters. Somehow passing it 3 parameters did not make PHP explode. So it was passing values like file folder in the position where originalPath should go.
false would always be returned, because the trashbin indeed did not have a file called file or a file called folder
| */ | ||
| public function theResponseShouldContainACustomPropertyWithValue( | ||
| $propertyName, $propertyValue, $table=null |
There was a problem hiding this comment.
Goodness knows what $table was doing here???
| @@ -701,7 +699,7 @@ public function theSingleResponseShouldContainAPropertyWithValueAndAlternative( | |||
|
|
|||
| if ($expectedValue === "a_comment_url") { | |||
| if (\preg_match("#^/remote.php/dav/comments/files/([0-9]+)$#", $value)) { | |||
| return 0; | |||
| return; | |||
There was a problem hiding this comment.
We just want to get out early here, no need to return ``0```
| @@ -743,9 +741,7 @@ public function theSingleResponseShouldContainAPropertyWithValueLike( | |||
| } | |||
| } | |||
|
|
|||
| if (\preg_match($regex, $value)) { | |||
| return 0; | |||
There was a problem hiding this comment.
No need to return at all here. We can just do the reverse test, and then we are at the end of the function anyway.
|
|
||
| // Return an invalid file id so that any later step that tries to use it | ||
| // will fail. | ||
| return ""; |
There was a problem hiding this comment.
Functions can return a value to Behat, which will remember the return value. Then it can be saved away for later, by a "built-in" step like:
And we save it into "FILEID"
So in this function we want to always return something, which will keep Behat happy. Then some later step in the scenario can fail gracefully when it tries to use an empty file id.
|
|
||
| $response = $client->proppatch( | ||
| $this->getDavFilesPath($user) . $path, $properties, $folderDepth |
There was a problem hiding this comment.
proppatch does not take a 3rd parameter. $folderDepth is irrelevant here. All callers specified 0 anyway.
| @@ -593,6 +603,7 @@ public function itShouldNotBePossibleToShareUsingTheWebUI( | |||
| 'could not find sharing button in fileRow', | |||
| 'could not share with \'' . $shareWith . '\'' | |||
| ]; | |||
| $foundMessage = false; | |||
There was a problem hiding this comment.
to keep the IDE happy - it thinks maybe $foundMessage could be undefined.
| @@ -1133,12 +1145,12 @@ public function adminMakesUserNotSubadminOfGroupUsingTheAPI($user, $group) { | |||
| /** | |||
| * @Then /^the users returned by the API should be$/ | |||
| * | |||
| * @param \Behat\Gherkin\Node\TableNode|null $usersList | |||
| * @param TableNode|null $usersList | |||
There was a problem hiding this comment.
so you can give the function a null but then it would do exactly nothing?
There was a problem hiding this comment.
I didn't look at that - the IDE just auto-simplified the TableNode reference for me.
Yes, you are right. If you pass nothing then the code here always passes - it does not check that the actual list is empty. There is a separate step for that:
the list of users returned by the API should be empty
so I will remove the null here.
And the same thing for groups below. I also see that there is no test scenario for getting the groups of a user when the user is not in any other groups - I will look at that separately.
|
Removed ```null`` return from functions that should not have it. |
phil-davis
force-pushed
the
acceptance-test-use-01
branch
from
d933fb8 to
0720641
Compare
|
Backport ``stable10``` #31536 |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
isInTrash``` that had too many/wrong parameters. The testelementIsNotInTrashCheckingOriginalPath`` always passed, now it could fail.proppatchcall inchangeFavStateOfAnElement- I don't think the test was actually broken.throwstags to loads of docblocks.Motivation and Context
Cleanup acceptance test code and let the IDE find dodgy-looking stuff.
How Has This Been Tested?
Local CI running.
Types of changes
Checklist: