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

Backport of #3459 to PHPUnit 6.5.x #3486

Merged
merged 1 commit into from
Jan 17, 2019
Merged

Backport of #3459 to PHPUnit 6.5.x #3486

merged 1 commit into from
Jan 17, 2019

Conversation

epdenouden
Copy link
Contributor

The fix for #3459 didn't help out @MaxSem who originally reported it: wikimedia/mediawiki@9c51746
As I'd like to support both Wikimedia and their security unit testing, I have manually backported and tested the fix, just this once. ;-)

image

It's just the @requires regex and its tests. Why Github says it cannot merge automatically, no idea. It works just fine when I try it:

 ~/proj/phpunit  6.5  git merge origin/issue-3459-fix-requires-annotation-phpunit65                                                                                          ✔  14:11:09 
Updating ea739aae4..96d3f0409
Fast-forward
 src/Util/Test.php                 |  2 +-
 tests/Framework/TestCaseTest.php  |  4 ++--
 tests/Util/TestTest.php           |  8 ++++----
 tests/_files/RequirementsTest.php | 12 ++++++++++--
 4 files changed, 17 insertions(+), 9 deletions(-)

[...]
PHPUnit 6.5.7-2-g96d3f0409 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.2.11 with Xdebug 2.6.0
Configuration: /Users/ewout/proj/phpunit/phpunit.xml

[...]
OK, but incomplete, skipped, or risky tests!
Tests: 1675, Assertions: 2930, Skipped: 2.

@sebastianbergmann
Copy link
Owner

Crazy Ivan that you are, you should have noticed that this branch has conflicts that must be resolved ;-)

On a slightly more serious note, though, the 6.5 branch is still supported for a couple of weeks. So there is nothing out of the ordinary to fix issues there.

@epdenouden
Copy link
Contributor Author

Well now I'm just annoyed. Fine. Put a bunch of TLAMs in tubes 3 to 6 and take us up to periscope depth. #coldwaters

@sebastianbergmann
Copy link
Owner

Well, Sir, I was just thinking that perhaps there's another possibility we might consider. @epdenouden might be trying to remove a defect.

@epdenouden
Copy link
Contributor Author

I do intend to fix it... if I managed to understand what's going on. :-) Damnit! It's a such simple fix, what did I do, why is programming computers so difficult.

@codecov
Copy link

codecov bot commented Jan 16, 2019

Codecov Report

Merging #3486 into 6.5 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##                6.5    #3486   +/-   ##
=========================================
  Coverage     80.14%   80.14%           
  Complexity     2854     2854           
=========================================
  Files           105      105           
  Lines          7529     7529           
=========================================
  Hits           6034     6034           
  Misses         1495     1495
Impacted Files Coverage Δ Complexity Δ
src/Util/Test.php 94.44% <ø> (ø) 198 <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 6050504...07029de. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jan 16, 2019

Codecov Report

Merging #3486 into 6.5 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##                6.5    #3486   +/-   ##
=========================================
  Coverage     80.14%   80.14%           
  Complexity     2854     2854           
=========================================
  Files           105      105           
  Lines          7529     7529           
=========================================
  Hits           6034     6034           
  Misses         1495     1495
Impacted Files Coverage Δ Complexity Δ
src/Util/Test.php 94.44% <ø> (ø) 198 <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 6050504...07029de. Read the comment docs.

@epdenouden
Copy link
Contributor Author

epdenouden commented Jan 16, 2019

@sebastianbergmann This was more a case of Hostile Waters with the code collisions. After retracing my steps and rebasing on the upstream/6.5 it works. :)

image
"Engineering reports everything back to green"

@sebastianbergmann sebastianbergmann merged commit 17a8efe into sebastianbergmann:6.5 Jan 17, 2019
@keradus
Copy link
Contributor

keradus commented Jan 17, 2019

Out of curiosity , as this is not first time a bug is backported.

Maybe let's have the policy to always send bugfixes against lowest maintained branch, on which bug exists?
That way, instead of sending same fix as different PRs to different targets, we would have send it once, to 6.5, then 6.5 would be merged to 7.x and that's it, all supported lines are bugfree now ?
That's what we do in PHP CS Fixer. Last supported version is 2.12, it collects most bugs, then we merge it to next supported version of 2.14.
What do you think, @sebastianbergmann ?

@epdenouden epdenouden deleted the issue-3459-fix-requires-annotation-phpunit65 branch January 17, 2019 10:59
@sebastianbergmann
Copy link
Owner

@keradus Agreed.

@keradus
Copy link
Contributor

keradus commented Jan 17, 2019

Awesome @sebastianbergmann , thanks ;)

@epdenouden
Copy link
Contributor Author

@keradus Thanks for bringing this up, I wasn't completely sure. Myself I prefer to target the oldest supported version out there as companies often run on older-and-proven stuff. As long as the older version has the basic infrastructure in place for the feature or bugfix to be included. (e.g. buffering printer does require the execution ordering helpers)

@epdenouden
Copy link
Contributor Author

@keradus Also, have there been any recent changes that you'd explicitly like to see included in pre-8 versions?

@keradus
Copy link
Contributor

keradus commented Jan 17, 2019

no, i was talking about policy in general how to treat bugfix releases, not about concrete bug.

@epdenouden
Copy link
Contributor Author

That's how I understood it. Since you (obviously :-) noticed a few backports going on, I thought you also might have a specific wish for some fix to go into an older version. Just checking.

@keradus
Copy link
Contributor

keradus commented Jan 17, 2019

less backports = less time wasted on it. just that ;)

@epdenouden
Copy link
Contributor Author

ONE MERGE ONLY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants