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

Allow testSendToGetLargeContent peak memory usage to be specified externally #187

Merged
merged 2 commits into from
Jul 15, 2022

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Jul 15, 2022

See discussion sabre-io/dav#1409 (comment)

The 'sabre/http' PHP unit tests are also run in the sabre/dav CI. The PHP environment is a bit "bigger" there, and memory_get_peak_usage() returns about 70MB, above the 60MB limit that the test allows.

This PR adds an env variable that can be defined to specify the peak memory usage that is allowed for the test. That will let us be more flexible in other CI.

I called it SABRE_HTTP_TEST_GET_LARGE_CONTENT_MAX_PEAK_MEMORY_USAGE - naming is hard, and I wanted a name that won't conflict with anything else in anyone's environment!

@phil-davis phil-davis self-assigned this Jul 15, 2022
tests/HTTP/ClientTest.php Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #187 (87fbcab) into master (87fbcab) will not change coverage.
The diff coverage is n/a.

❗ Current head 87fbcab differs from pull request most recent head 94321c1. Consider uploading reports for the commit 94321c1 to get more accurate results

@@            Coverage Diff            @@
##             master     #187   +/-   ##
=========================================
  Coverage     94.11%   94.11%           
  Complexity      260      260           
=========================================
  Files            15       15           
  Lines           884      884           
=========================================
  Hits            832      832           
  Misses           52       52           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@phil-davis phil-davis requested a review from staabm July 15, 2022 10:44
@phil-davis phil-davis marked this pull request as ready for review July 15, 2022 10:44
@phil-davis
Copy link
Contributor Author

And I think that I will have to make a patch release so that sabre/dav CI will find and use this test change. That's a little bit sad for the real consumers of this repo - the change is test-only and actually does not fix or alter any real behavior.

tests/HTTP/ClientTest.php Outdated Show resolved Hide resolved
@phil-davis phil-davis merged commit 4cb5502 into sabre-io:master Jul 15, 2022
@phil-davis phil-davis deleted the testSendToGetLargeContent branch July 15, 2022 14:10
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.

2 participants