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

Update travis.yml - add pypy compiler test env #129

Closed
wants to merge 3 commits into from

Conversation

phanak-sap
Copy link
Contributor

@phanak-sap phanak-sap commented Oct 9, 2020

@phanak-sap
Copy link
Contributor Author

Build is failing due #130

@phanak-sap phanak-sap added the wip label Jan 21, 2021
@phanak-sap phanak-sap marked this pull request as draft January 21, 2021 12:46
@phanak-sap phanak-sap self-assigned this Jul 21, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2022

Codecov Report

Merging #129 (e9e6375) into master (ec56995) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #129   +/-   ##
=======================================
  Coverage   92.66%   92.66%           
=======================================
  Files           6        6           
  Lines        2768     2768           
=======================================
  Hits         2565     2565           
  Misses        203      203           

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 ec56995...e9e6375. Read the comment docs.

@phanak-sap
Copy link
Contributor Author

phanak-sap commented Jan 14, 2022

Build are now working on Travis, valid test failures - in all LXML + python version seems to be same

======================== 4 failed, 228 passed in 30.91s ========================

AttributeError: '_io.BytesIO' object has no attribute '__sizeof__'

Additional note. Pypy3 coverage is experimental, just to be known if it works, should be marked as allow_failures in build matrix, even when the test will be fixed. Standard Python must pass, pypy should not block the PRs.

@filak-sap
Copy link
Contributor

So you want to merge and expect that people know that pypy3 can fail?

@phanak-sap
Copy link
Contributor Author

phanak-sap commented Jan 17, 2022

So you want to merge and expect that people know that pypy3 can fail?

I do not know how can I be more specific than what's already stated in the PR. Nothing in this sentence is true. It is entirely the opposite.

  1. this PR is marked as Draft. I do not want to merge it at the moment.
  2. to be possible to merge, I obviously need to fix those 4 tests, ( it seems they have same root cause). I do not want to introduce already failing build.
  3. even if this PR will have 100 percent PASS rate on entire build matrix, the pypy3 passing for every other future PR still should be optional. This is noted by previous comment "Pypy3...should be marked as allow_failures in build matrix, even when the test will be fixed. Standard Python must pass, pypy should not block the PRs.", This means, in more words, that even after this is merged, .travis.yaml needs to have this build job marked by its directive allow_failures - meaning in effect, that a) all python versions for future PRs must pass b) passing on pypy will be still optional, until stated otherwise. Any failures detected on CI will be just issues on me, until proven that the pypy is worth the effort. Then, maybe, in some future, pypy environment will be marked as mandatory to pass on every PR - if the maintenance effort will be lower than the benefits from speed gain. Note: even that is stated that pypy is on average quicker than cpython, there should be for sure some performance testing before such enforcement.

@phanak-sap
Copy link
Contributor Author

We use Github Actions only now, so closing with follow-up in #237

@phanak-sap phanak-sap closed this Oct 3, 2022
@phanak-sap phanak-sap deleted the phanak-travis-pypy branch October 3, 2022 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants