Try to fix GitHub Actions

[simonhammes]

Pull Request Type

• I have checked there is no other PR open for the same change.

This is a:

• Bug fix
• New feature
• Code quality improvement

Detailed Description

The issue in https://github.com/WordPress/Requests/runs/4614423459?check_suite_focus=true might be related to composer/composer#10389, which was fixed/released in https://github.com/composer/composer/releases/tag/2.2.4.

Unfortunately I could not get the tests to run locally, but the error message disappeared after downgrading to Composer 2.1.14.

I am actually trying to work on #475 and maybe add a Dockerfile and a shell script to the repository to make it easier to run the tests locally.

Quality assurance

• This change does NOT contain a breaking change (fix or feature that would cause existing functionality to change).
• I have commented my code, particularly in hard-to-understand areas.
• I have added unit tests to accompany this PR.
• The (new/existing) tests cover this PR 100%.
• I have (manually) tested this code to the best of my abilities.
• My code follows the style guidelines of this project.

Documentation

For new features:

• I have added a code example showing how to use this feature to the examples directory.
• I have added documentation about this feature to the docs directory.
  If the documentation is in a new markdown file, I have added a link to this new file to the Docs folder README.md file.

[simonhammes]

Yay, I got the tests passing using Composer 2.1.14.

It seems like Composer 2.2.4 did not fix the issue.

Another solution for this problem would be to modify https://github.com/RequestsPHP/test-server/blob/e9311dc36fa95e9f0f4077cdccbc8aede84f2199/bin/start.sh#L1 from SERVERDIR="$PWD/$(dirname $0)" to SERVERDIR="$(dirname $0)".

Or maybe we could copy the necessary files from https://github.com/RequestsPHP/test-server into this repository to avoid such problems alltogether?

EDIT: Or apply a patch using https://github.com/cweagans/composer-patches.

[jrfnl]

Hi @simonhammes, thank you for looking into this!

I'd like to take a few steps back though and look at the various things you are saying one by one.

I am actually trying to work on #475 and maybe add a Dockerfile and a shell script to the repository to make it easier to run the tests locally.

That's awesome! Please be aware though that Docker is no longer free for all, so may not be a sustainable solution. Also please be aware that people contributing to this library may be on a variety of OSes for their dev environment, so shell scripts would need to take that into account.

Unfortunately I could not get the tests to run locally

Did you not get the tests to run at all locally or did most tests run with a select number of tests being skipped/failing ?
How are you trying to run the tests ?

but the error message disappeared after downgrading to Composer 2.1.14.

While that may be a temporary work-around, it is not a solution and I would very much prefer a proper solution over a work-around which will bite us in the ass later on.

EDIT: Or apply a patch using https://github.com/cweagans/composer-patches.

Same thing. I'd very much would like to avoid adding extra dependencies to work around a bug which should be fixed properly instead.

The issue in https://github.com/WordPress/Requests/runs/4614423459?check_suite_focus=true might be related to composer/composer#10389, which was fixed/released in https://github.com/composer/composer/releases/tag/2.2.4.

It seems like Composer 2.2.4 did not fix the issue.

Nice find! Composer 2.2 has turned out to be quite problematic all around so far, I've been involved in two other issues myself so far.

As that issue is closed, it may be worthwhile asking to reopen it it or to open a new, follow-up issue in the Composer repo outlining the problem you identified here.

Another solution for this problem would be to modify https://github.com/RequestsPHP/test-server/blob/e9311dc36fa95e9f0f4077cdccbc8aede84f2199/bin/start.sh#L1 from SERVERDIR="$PWD/$(dirname $0)" to SERVERDIR="$(dirname $0)".

I'm not sure that particular change would be the most stable cross-platform solution... ?

Or maybe we could copy the necessary files from https://github.com/RequestsPHP/test-server into this repository to avoid such problems alltogether?

Actually, I think we should probably ask @rmccue if he's willing to give @schlessera and me admin access to the whole RequestsPHP organization as the test server repo is the only repo in it anyway.

Come to think of it, moving this repo to that organisation with Alain and me as (extra) admins, might have been the better choice instead of the move to the WP organisation last year....

Once we have maintainer access to the test server repo, giving the code there a good once-over and tech debt review would be great! That repo has barely been touched for the past five/six years, so there's bound to be some touching up needed. Having access to that repo and being able to maintain it, will also help with a few test cases for this repo which need to be added as it will allow us to generate additional server responses needed for those tests cases.

[jrfnl]

@rmccue If you have a chance, it would be great to get your input on the above proposal for adding @schlessera and me to the Test server repo...

[simonhammes]

Hi @simonhammes, thank you for looking into this!

@jrfnl Thank you for getting back to me so quickly!

Unfortunately I could not get the tests to run locally

Did you not get the tests to run at all locally or did most tests run with a select number of tests being skipped/failing ? How are you trying to run the tests ?

Some were failing, although I got all of them passing now. That also allowed me to open #667.
I will soon work on updating #278 with all relevant details on how to run the tests locally (without Docker or any extra tools).
However, this requires Composer < 2.2 right now.

While that may be a temporary work-around, it is not a solution and I would very much prefer a proper solution over a work-around which will bite us in the ass later on.

You are correct.

---

I will close this PR since Composer 2.2.5 did not fix the issue and we should find a stable solution to this problem.
The comment in composer/composer#10389 (comment) also sounds like there won't be any more fixes concerning this problem.

[rmccue]

@rmccue If you have a chance, it would be great to get your input on the above proposal for adding @schlessera and me to the Test server repo...

@jrfnl Invited you and @schlessera as owners :)

FWIW: the test app is hosted on a deprecated version of the Heroku platform. It might be possible to move these to Docker containers (or GH Actions) instead, but it was intentionally cloud-hosted to ensure it goes through a full HTTP and networking stack, and that things like HTTP proxies are actually working as desired. I used Heroku specifically because it passed through the data without interfering, other platforms tended to mess with the input/output data/headers or fail when passed semi-invalid data.

(Happy to add you as collaborators on Heroku as well if desired, will need your Heroku account emails I think.)

[jrfnl]

Thanks for that @rmccue ! I don't have a Heroku account myself. Maybe @schlessera has one ?

I have a niggly feeling that we're currently running the test app directly in GH Actions already, but would need to verify this to be sure.

[rmccue]

Ah, I think actually the issue was that they need to run behind a real server so that it exercises HTTPS tests against the real CA stack. We use the domain in the tests to set the constants:

Requests/tests/bootstrap.php

Lines 14 to 16 in 73dbbbe

	define_from_env('REQUESTS_TEST_HOST', 'requests-php-tests.herokuapp.com');
	define_from_env('REQUESTS_TEST_HOST_HTTP', REQUESTS_TEST_HOST);
	define_from_env('REQUESTS_TEST_HOST_HTTPS', REQUESTS_TEST_HOST);

The test runner only overrides the HTTP version, not HTTPS:

Requests/.github/workflows/test.yml

Line 88 in 73dbbbe

echo "REQUESTS_TEST_HOST_HTTP=localhost:8080" >> $GITHUB_ENV

This host is only used for calls to httpbin( ..., true ), which (according to GH code search) is only used in testHTTPS:

Requests/tests/Transport/BaseTestCase.php

Line 833 in 73dbbbe

$request = Requests::get(httpbin('/get', true), [], $this->getOptions());

[schlessera]

@rmccue We've fixed the code now. Would it be possible for you to add me to the Heroku app as a manager, so I can verify its state and push updates? I have a Heroku account under the email alain.schlesser@gmail.com.

[rmccue]

@schlessera Added!

[jrfnl]

Thanks @rmccue !