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

Improve export of closed resources #37

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Nov 11, 2021

Fixes #36

If you're so inclined, a backport to the 4.x branch would be great! ❤️

Note: I've added a second commit to fix the CI, which was failing on the composer install with the recursive dependency. The fix as now included should work during the whole of the 5.x dev cycle and will need adjusting when work starts on the next major (after 5.x).

@codecov
Copy link

codecov bot commented Nov 11, 2021

Codecov Report

Merging #37 (9b934e8) into 4.0 (b4bd132) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                4.0      #37      +/-   ##
============================================
+ Coverage     98.09%   98.13%   +0.03%     
- Complexity       42       43       +1     
============================================
  Files             1        1              
  Lines           105      107       +2     
============================================
+ Hits            103      105       +2     
  Misses            2        2              
Impacted Files Coverage Δ
src/Exporter.php 98.13% <100.00%> (+0.03%) ⬆️

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 b4bd132...9b934e8. Read the comment docs.

@sebastianbergmann
Copy link
Owner

If you're so inclined, a backport to the 4.x branch would be great! ❤️

Please send this against the oldest supported branch and I will gladly forward-port it from there.

@sebastianbergmann
Copy link
Owner

Note: I've added a second commit to fix the CI, which was failing on the composer install with the recursive dependency. The fix as now included should work during the whole of the 5.x dev cycle and will need adjusting when work starts on the next major (after 5.x).

This " composer install with the recursive dependency" issue confuses me: I cannot reproduce it locally, I only see it in CI. Can you (or @localheinz, or maybe even @Seldaek) enlighten me as to why that is? Thanks!

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 11, 2021

Please send this against the oldest supported branch and I will gladly forward-port it from there.

Thanks @sebastianbergmann. The thing is... this repo doesn't seem to have a 4.x branch... The branches I can see are 1.2, 2.0, 3.1 and master. When you say "oldest supported branch", I don't suppose you mean the 1.2 branch ?

I'll happily rebase the PR and add support for recognizing closed resources all the way back to 1.2 if you like, but I just want to make sure that's what you actually mean.

As for the (non-existent) 4.x branch: I don't know which commits between the 4.0.3 tag and the current master belong with 4.x and which with 5.x. My best guess would be to take commit 1f81962 as 4.x as it is the last commit before the version drop.

@sebastianbergmann
Copy link
Owner

sebastianbergmann commented Nov 11, 2021

PHPUnit 8.5 uses version 3.1 of this library, so the 3.1 branch should be the target for this patch. From there I shall port it to 4.0 (used by PHPUnit 9.5) and master (used by PHPUnit 10).

The 4.0 branch did not exist until a few moments ago, sorry about that.

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 11, 2021

Thanks! Will sort it out later today (after my current call).

@jrfnl jrfnl force-pushed the feature/improve-information-on-closed-resource branch from 773b671 to dfc0b1c Compare November 11, 2021 11:08
@jrfnl jrfnl changed the base branch from master to 4.0 November 11, 2021 11:09
@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 11, 2021

I've rebased the branch on 4.0 and have changed the base branch for this PR. As a slightly different solution is needed for the 3.x branch, I've opened a separate pull requests with the fix for 3.x. (#38)

@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 11, 2021

Note: I've added a second commit to fix the CI, which was failing on the composer install with the recursive dependency. The fix as now included should work during the whole of the 5.x dev cycle and will need adjusting when work starts on the next major (after 5.x).

This " composer install with the recursive dependency" issue confuses me: I cannot reproduce it locally, I only see it in CI. Can you (or @localheinz, or maybe even @Seldaek) enlighten me as to why that is? Thanks!

@sebastianbergmann I've run into this before and had to do some digging to find the link on which I based the fix: https://twitter.com/seldaek/status/1299582760781307904

The short of it is, that with branch_alias and full git history, things will locally work fine most of the time, but CI doesn't always have full history, which is why the additional COMPOSER_ROOT_VERSION is needed to get round the recursive dependency in CI.

Also: https://getcomposer.org/doc/03-cli.md#composer-root-version

@@ -6,6 +6,10 @@ on:

name: "CI"

env:
# - COMPOSER_ROOT_VERSION is needed to get round the recursive dependency when using CI.
COMPOSER_ROOT_VERSION: '4.99.99'
Copy link
Contributor

Choose a reason for hiding this comment

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

@jrfnl @sebastianbergmann

Could also be resolved somewhat automatically, see https://github.com/ergebnis/composer-root-version-action.

@jrfnl jrfnl force-pushed the feature/improve-information-on-closed-resource branch from dfc0b1c to 5654843 Compare November 11, 2021 11:35
@jrfnl jrfnl force-pushed the feature/improve-information-on-closed-resource branch from 5654843 to 9b934e8 Compare November 11, 2021 11:37
@jrfnl
Copy link
Contributor Author

jrfnl commented Nov 11, 2021

P.S.: The new pushes are nothing exciting. I just noticed I'd missed adding the use function fclose; to the test file, so fixed that.

@sebastianbergmann sebastianbergmann merged commit c4ad286 into sebastianbergmann:4.0 Nov 11, 2021
@jrfnl jrfnl deleted the feature/improve-information-on-closed-resource branch November 11, 2021 13:16
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.

Improve export of closed resources
3 participants