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

Fix Travis CI #302

Merged
merged 10 commits into from
Dec 13, 2017
Merged

Fix Travis CI #302

merged 10 commits into from
Dec 13, 2017

Conversation

ntwb
Copy link
Member

@ntwb ntwb commented Dec 12, 2017

This is a WIP to fix Travis CI - Do Not Merge Yet 🤖

Cherry picking various pieces from #298 and #280

@codecov-io
Copy link

codecov-io commented Dec 12, 2017

Codecov Report

Merging #302 into master will decrease coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #302      +/-   ##
============================================
- Coverage     92.11%   91.94%   -0.18%     
- Complexity        0      760     +760     
============================================
  Files            21       21              
  Lines          1762     1762              
============================================
- Hits           1623     1620       -3     
- Misses          139      142       +3
Impacted Files Coverage Δ Complexity Δ
library/Requests/Proxy/HTTP.php 86.66% <0%> (-13.34%) 13% <0%> (+13%)
library/Requests/Transport/fsockopen.php 94.76% <0%> (+0.58%) 68% <0%> (+68%) ⬆️

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 87932f5...cdca915. Read the comment docs.

@ntwb
Copy link
Member Author

ntwb commented Dec 12, 2017

This "could" be merged now, at least the PHPUnit failures are now visible, fixing those should get passing Travis CI jobs for PHP 5.3 - PHP 7.2

I haven't even looked at any HHVM issues, I expect them to fail and should more than likely be removed, though I'm not sure who else apart from WordPress is using this library

PHP 5.2 is broken, because Composer requires >PHP 5.3, I can follow up fixes for PHP 5.2 in a subsequent PR.

@ntwb
Copy link
Member Author

ntwb commented Dec 12, 2017

If I can get someone to approve this I'll merge it 🎉

- composer install --dev --no-interaction
- TESTPHPBIN=$(phpenv which php)
- phpenv local --unset
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does composer install work on PHP 5.2 though? That was the issue previously which required the phpenv hack.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not, I'll add back Composer for PHP 5.2 in a follow-up PR, it was quicker and easier to have some CI functionality for the majority of PHP versions rather than nothing, now all but PHP 5.2 and HHVM work, previously all CI jobs were erroring

@rmccue
Copy link
Collaborator

rmccue commented Dec 12, 2017

I'm hesitant to merge a PR that doesn't actually fix the tests. Based on the failures I see on https://travis-ci.org/rmccue/Requests/jobs/315173786, it seems like the local HTTP server isn't actually running, which I suspect is due to removing the phpenv lines.

@ntwb
Copy link
Member Author

ntwb commented Dec 12, 2017

That’s fair enough, I hadn’t dug in any further than confirming PHPUnit tests were now running (previously they were not)

I’m also pretty unfamiliar with this library, I’ll take a look at what the HTTP server bits look like and take a stab at fixing that, and PHP 5.2

As to fixing the bugs reported I’ll be leaving that others cc @dd32

@dd32
Copy link
Member

dd32 commented Dec 12, 2017

Looks like the proxy failure is due to mitmproxy not supporting Python 2:

Error: mitmproxy requires Python 3.5 or above
Starting with version 1.0 released in 12/2016, mitmproxy no longer supports Python 2.
To install the latest version of mitmproxy using pip on Python 3, run:
    pip uninstall mitmproxy
    pip3 install mitmproxy
You can also use our standalone binaries that come with their own Python interpreter:
    http://docs.mitmproxy.org/en/stable/install.html
To get rid of this message and use the last version of mitmproxy that supports Python 2, run:
    pip install "mitmproxy==0.18.2"
Apologies for the inconvenience!`

@dd32
Copy link
Member

dd32 commented Dec 12, 2017

I took a further swing at it, here's the green result: https://travis-ci.org/dd32/Requests/builds/315356440 from https://github.com/dd32/Requests/tree/pr/302 specifically https://github.com/dd32/Requests/commit/3bbcd2bd55b0f29a42ebb6c886201f24e10b6a1e, https://github.com/dd32/Requests/commit/80291d262501acbe87b36284c74409e88e9e39e4, https://github.com/dd32/Requests/commit/4b91d503f025adde368d1cd6b48476660f22c56a, and https://github.com/dd32/Requests/commit/6493d638d901a9ecc06bc9715e5f2fd2cd6e4944 (I can't push to the PR here)

  • MITMProxy was failing as 1.0+ requires Python3, seemed easier to just pin it to the last release which supports Python2. This also required fixing the proxy.py script, I couldn't see what version the syntax of the callbacks changed.
  • phpenv local was failing on precise as it seems that phpenv doesn't have the X.Y -> X.Y.Z symlinks created for PHP 5.5/5.6 (only for 5.2/5.4). Instead i dynamically pulled the version in (5.6.13/5.6.31 on precise/trusty)
  • HHVM failed because of silly PHPUnit version compares, I just dropped the HHVM testing entirely https://github.com/dd32/Requests/commit/4b91d503f025adde368d1cd6b48476660f22c56a (see https://travis-ci.org/dd32/Requests/jobs/315315609) If you want to keep it @rmccue it's possible, but doesn't seen worth anyones time.
  • Tests fail occasionally on the following test, but that's unrelated to the issue at hand, restarting the job fixes that.
RequestsTest_Transport_cURL::testMultipleWithFailure
Undefined property: Requests_Exception_Transport_cURL::$status_code
/home/travis/build/dd32/Requests/tests/Transport/Base.php:682

@ozh
Copy link
Collaborator

ozh commented Dec 12, 2017

@dd32 you rock. I totally gave up on phpenv when trying to fix. I completely agree on HHVM.

So, what's next ? We close here and you open a new PR to merge it?

@rmccue
Copy link
Collaborator

rmccue commented Dec 13, 2017

@dd32 Thanks for that! Let's PR that up and get it merged.

HHVM failed because of silly PHPUnit version compares

Happy to drop compatibility with it now that they're no longer aiming for PHP compatibility.

@dd32 dd32 mentioned this pull request Dec 13, 2017
@rmccue rmccue merged commit cdca915 into WordPress:master Dec 13, 2017
@jrfnl jrfnl added this to the 1.7.1 milestone Sep 17, 2019
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.

6 participants