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.

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

php unit test changes to address issues that php7.4 will report #36586

Merged
merged 9 commits into from
Apr 7, 2020

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Dec 13, 2019

I am finding things in the unit tests that PHP7.4 complains about (PR #36500 )
Also put them here to see that the fixes/changes still pass with an older version(s) of PHP 7.1 7.2 7.3

  1. Fix 'Function ReflectionType::__toString() is deprecated' - in ControllerMethodReflector.php we will not be able to "just cast to string", use the getName() method.
  2. Fix 'stripos(): Non-string needles will be interpreted as strings in the future - $pattern might look like something other than a string. Cast it to string because we really want it to be a string.
  3. Fix 'Trying to access array offset on value of type bool' - the file called "hack" that was being put in the cache is sometimes cleaned out of the cache already by the test and the attempt to instance->remove('hack') complained because actually hack does not always exits. Only remove it if it still exists.
  4. Pass real core.key core.crt in SignAppTest and SignCoreTest to fix 'Trying to access array offset on value of type bool' - the mocking was not actually reading in the test key and crt files. Somehow the test was passing with the privateKey being literally privateKey and the certificate being the literal string certificate
  5. And similar stuff in other files.

@phil-davis phil-davis self-assigned this Dec 13, 2019
@codecov
Copy link

codecov bot commented Dec 13, 2019

Codecov Report

Merging #36586 into master will not change coverage by %.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #36586   +/-   ##
=========================================
  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% <100.00%> (ø) 19146.00 <0.00> (ø)
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Cache/Cache.php 97.64% <ø> (ø) 106.00 <0.00> (ø)
...AppFramework/Utility/ControllerMethodReflector.php 100.00% <100.00%> (ø) 10.00 <0.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...0434cca. Read the comment docs.

@phil-davis phil-davis force-pushed the phpunit-php7.4-stuff branch from a2da02b to 83044a0 Compare January 8, 2020 04:19
@phil-davis phil-davis force-pushed the phpunit-php7.4-stuff branch 2 times, most recently from 18256b4 to 388c14e Compare January 20, 2020 03:10
@phil-davis phil-davis marked this pull request as ready for review January 20, 2020 03:11
@phil-davis
Copy link
Contributor Author

@DeepDiver1975 or whoever cares to give review/feedback on this - these are things that are reported by phpunit when running PHP 7.4, e.g. see PR #36670

Does the way I have adjusted things in this PR make sense?

If so, then we could merge this, and then "someone" can be tasked to adjust the rest of the issues "someday".

@phil-davis phil-davis force-pushed the phpunit-php7.4-stuff branch 2 times, most recently from 98ef2aa to 81ca245 Compare January 28, 2020 11:07
@phil-davis phil-davis requested a review from micbar January 28, 2020 11:07
@phil-davis
Copy link
Contributor Author

@DeepDiver1975 @micbar or anybody - is this the right way to adjust these unit tests?

If so, then this is an example for when we really get working on PHP 7.4 support.

It could be merged now - this all works with PHP 7.1 7.2 7.3

@phil-davis phil-davis force-pushed the phpunit-php7.4-stuff branch from 81ca245 to 15268b3 Compare February 7, 2020 08:53
@phil-davis
Copy link
Contributor Author

Rebased just now to get new .drone.star stuff.

@phil-davis
Copy link
Contributor Author

@jvillafanez here is some changes for supporting PHP 7.4. This seems to be the sort of stuff we need to do. PR #37004 runs CI with PHP 7.4 to see what it reports (there are plenty more things to adjust)

@phil-davis phil-davis force-pushed the phpunit-php7.4-stuff branch from 0c1b77a to 9b69c43 Compare March 10, 2020 13:57
@phil-davis phil-davis force-pushed the phpunit-php7.4-stuff branch from 9b69c43 to 0ccd1ee Compare March 18, 2020 04:38
@phil-davis phil-davis force-pushed the phpunit-php7.4-stuff branch 2 times, most recently from 767fa78 to 6f4776d Compare March 27, 2020 05:14
@phil-davis
Copy link
Contributor Author

@micbar @DeepDiver1975 @jvillafanez @VicDeo or anyone.
This can be reviewed to see if there is agreement about what I have done here.
It all works fine with current PHP 7.1 7.2 7.3 core master. So it could be merged.
And then when somebody allocates resources in the future for PHP 7.4 support, it will be an example of changes, for the next developer to work through whatever other unit test warnings... need to be fixed.

@phil-davis phil-davis requested a review from VicDeo March 27, 2020 05:19
@phil-davis phil-davis changed the title Phpunit php7.4 stuff php unit test changes to address issues that php7.4 will report Mar 27, 2020
@jvillafanez
Copy link
Member

The cleanup in the cache should be moved to the tear down function, but taking into account the previous code, I'm fine with these changes. 👍

@phil-davis phil-davis mentioned this pull request Mar 28, 2020
@phil-davis phil-davis force-pushed the phpunit-php7.4-stuff branch from 6f4776d to 0434cca Compare April 7, 2020 04:16
@phil-davis
Copy link
Contributor Author

The cleanup in the cache should be moved to the tear down function, but taking into account the previous code, I'm fine with these changes. +1

@jvillafanez did you mean that I can leave the code as I have written it, or that you want me to refactor code into the teardown function?

@jvillafanez
Copy link
Member

Up to you. Technically, this PR is to fix php 7.4 issues, so the tear down problem is out of the scope. You can open a new PR to fix that problem, or if it's easier for you (and the amount of changes are controlled) you can fix it here.

@phil-davis
Copy link
Contributor Author

phil-davis commented Apr 7, 2020

The code like:

$this->cache->remove($dir1);

Is followed by existing asserts like:

$this->assertFalse($this->cache->inCache($dir1));

So the cache remove code is not "just" cleanup at the moment.

That could all be sorted out (e.g. why are those assertFalse there? Do they really do some useful check related to the test case?) But IMO that is for some other PR.

@micbar @DeepDiver1975 @jvillafanez @VicDeo or anyone. Please review.

@phil-davis phil-davis merged commit dfb9985 into master Apr 7, 2020
@delete-merged-branch delete-merged-branch bot deleted the phpunit-php7.4-stuff branch April 7, 2020 11:04
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