Refactoring tests

[carusogabriel]

Simple version of #6853, just using assertNull, assertFileExists, assertTrue and assertFalse. Also, in develop branch.

[lcobucci]

Awesome, LGTM 👍

[lcobucci]

Since @guilhermeblanco is rebasing develop I'll leave this to him.

[carusogabriel]

I've improved more tests, using:

• assertCount instead of count and sizeof PHP function;
• assertInternalType instead of is_* PHP functions;
• assertEquals instead of comparisons == and assertSame instead of strict comparasions ===;
• assertStringEqualsFile when comparing a string and a file;
• assertTrue or assertFalse instead of comparisons with true or false keyworkds with assertEquals/assertSame;
• assertGreaterThan for mathamatical comparisons;
• assertContains instead of in_array PHP function;
• assertInstanceOf and assertNotInstanceOf instead of strict comparisons with instanceof PHP operator;
• assertArrayHasKey instead of isset function.

[Majkl578]

This change is incorrect.

[carusogabriel]

I’m trying to rebase, but I guess I lost my work 😭

[Majkl578]

You've probably just incorrectly rebased, but your commits still exist: 6999d39 + 60cd9cf.
Hint: Sometimes it's easier to just hard-reset local copy and re-apply the commits again by cherry-picking them.

[carusogabriel]

I’m gonna try this, thanks for the tip

[carusogabriel]

Should we refactory assertEquals('', $foo)/assertSame('', $foo) with assertEmpty($foo), and assertEquals([], $bar)/assertSame([], $bar) with assertEmpty($bar)?

[Ocramius]

Don't think so: `assertEmpty()` is too lax

…

On 4 Dec 2017 08:43, "Gabriel Caruso" ***@***.***> wrote: Should we refactory assertEquals('', $foo)/assertSame('', $foo) with assertEmpty($foo), and assertEquals([], $bar)/assertSame([], $bar) with assertEmpty($bar)? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6854 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakBgfjEl4rbQy42VqCTI32lLcoHTpks5s86KNgaJpZM4QtVJ_> .

[Majkl578]

Develop has already been rebased so this can be merged now.

[Majkl578]

@carusogabriel Thanks!

[carusogabriel]

Any time @Majkl578 👨‍💻

[carnage]

Took a quick look at back porting this one and well, lets just say it's potentially going to be a big job.

94 out of 96 files have conflicts, a 95th is a deleted/modified conflict.

One of the biggest things I've noticed is there are conflicts on a lot of lines where $this->assert* has been changed to self::assert* (technically, the correct way to call the asserts) however this wasn't done in this PR. My guess is that we're missing a PR somewhere that did all the changes from $this-> to self:: and that finding and merging that one first might reduce the number of conflicts in this PR or at the very least make it possible to see what's going on in the diffs.

It is of cause possible that that PR will also have a huge number of conflicts, so it might be prudent to redo the work to change $this-> to self:: as it can be automated by php cs fixer.

Alternatively, if it's the project's choice to use $this-> to call the asserts php cs fixer could be run it in the opposite direction against the changes in this PR to make it closer to the current codebase before a merge is attempted.

[greg0ire]

I think redoing the change would be best, because you would not catch all occurrences anyway. We use phpcs in this project, maybe we can find a rule to add to our coding standard?

[carnage]

This is the rule class for php cs fixer https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/3.0/src/Fixer/PhpUnit/PhpUnitTestCaseStaticMethodCallsFixer.php but I can't find an equivalent in php codesniffer. We'd also need to decide which style to support (options being Assert::assertX, static::assertX, self::assertX and $this->assertX with the latter two being the usual choices)

[greg0ire]

🤔 I wonder why it is a phpunit-specific rule. My personal preference would be to run it once and make a PR, and pick self, because it won't conflict but because if we already merged that in the old-prototype-3.x, then there must be some consensus on using that.

[carnage]

In case anyone else comes across this ticket as a TODO, I have started work on it; however there are still ~60 conflicted files to go through so it's going to take a while :)