Skip to content

Update Travis to use tox, and add Appveyor CI #293

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

Closed
wants to merge 1 commit into from

Conversation

jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Jul 27, 2016

Update tox.ini to utilise requirements-test.txt, and run pylint.
Require lxml 3.6.0 on Windows as it has wheels available for 2.6-3.4.

Also enables coverage for PyPy on Travis.

Update tox.ini to utilise requirements-test.txt, and run pylint.
Require lxml 3.6.0 on Windows as it has wheels available for 2.6-3.4.

Also enables coverage for PyPy on Travis.
@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 27, 2016

Note requirements-install.sh could also be deleted.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 27, 2016

https://ci.appveyor.com/project/jayvdb17618/html5lib-python/build/1.0.22 is an Appveyor CI build of this commit (23c9013).

@codecov-io
Copy link

Current coverage is 90.46% (diff: 100%)

Merging #293 into master will decrease coverage by 0.01%

@@             master       #293   diff @@
==========================================
  Files            50         50          
  Lines          6930       6931     +1   
  Methods           0          0          
  Messages          0          0          
  Branches       1336       1338     +2   
==========================================
  Hits           6270       6270          
  Misses          500        500          
- Partials        160        161     +1   

Powered by Codecov. Last update a3022dc...23c9013

@jayvdb jayvdb mentioned this pull request Jul 28, 2016
@gsnedders
Copy link
Member

So the reason why we currently don't run tox on Travis is because I'm reluctant to make tox always run coverage: it has a notable performance hit on test execution. Given as far as I'm aware you have a better idea about tox than me—is there any decent way to avoid this? I guess one option is to use {postargs} and pytest-cov?

Other comments soon to be inline…

lxml ; platform_python_implementation == 'CPython'
lxml ; platform_python_implementation == 'CPython' and sys_platform != 'win32'
lxml==3.6.0 ; platform_python_implementation == 'CPython' and sys_platform == 'win32' and python_version <= '3.4'
lxml==3.6.0 ; platform_python_implementation == 'CPython' and sys_platform == 'win_amd64' and python_version <= '3.4'
Copy link
Member

Choose a reason for hiding this comment

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

why do we have this pinned to 3.6.0? (answers preferably as a comment!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not mandatory, but highly recommended to simplify life for Windows users.
My reasoning in the commit msg is:

Require lxml 3.6.0 on Windows as it has wheels available for 2.6-3.4.

3.6.1 doesnt have wheels; if we dont pin it, pip will attempt to install 3.6.1 from source.

If you agree with this reasoning, ill add a comment explaining it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, please do add a comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is all unnecessary now with later releases of lxml. yay. #363

@jayvdb
Copy link
Contributor Author

jayvdb commented Aug 5, 2016

I've got an idea on how make coverage optional, used on travis but not required on local dev usage.


pylint==1.6.4 ; python_version > '2.6'
pylint==1.3.1 ; python_version <= '2.6'
astroid==1.3.5 ; python_version <= '2.6'
Copy link
Member

Choose a reason for hiding this comment

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

What's this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to get Python 2.6 builds to work , as pylint dropped support for Python 2.6 a while ago, hence this workaround.
Anyway, #330 has landed, so that is unnecessary now.

@gsnedders
Copy link
Member

@jayvdb That idea is…? :)

@willkg willkg added this to the 1.0 milestone Oct 3, 2017
@willkg
Copy link
Contributor

willkg commented Oct 31, 2017

@jayvdb Seems like there's some work that needs to be done here. Further, over time some of the changed files themselves have changed and there are non-trivial merge conflicts.

Do you still want to land this? If so, do you have time to work on it?

@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 1, 2017

I'll take a look at it today to see if it is worth the effort ;-)

@willkg
Copy link
Contributor

willkg commented Nov 1, 2017

@jayvdb Thank you! I appreciate it!

@jayvdb jayvdb mentioned this pull request Nov 8, 2017
@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 8, 2017

IMO the most important part was getting AppVeyor doing Windows testing, so that part is moved to #359 .

I'll stay engaged in getting that patch merged.

Then regarding this patch, lots of parts can be dropped now, but there is still a nugget or two to be extracted, and I'll still keen to get that done. Next week ok?

@willkg
Copy link
Contributor

willkg commented Nov 8, 2017

@jayvdb The premise of this PR is important. We definitely want to switch to tox for managing test environments.

I've got some time this week to work on it, so I think I'll pull the bits I want from this PR and throw it in a new PR. Then I'll ping you to review that to make sure I got the bits you wanted done. Would that be ok?

@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 8, 2017

If it is important, I'll work on it sooner. I'd like to finish this off, if that is OK. (and yes, I'll probably splitting bits off to new PRs, and then kill this one when its all been rescued.)

@willkg
Copy link
Contributor

willkg commented Nov 8, 2017

@jayvdb I'm totally game for you doing it. It sounded like you didn't have time and I happen to have some time this week. Earlier is better, but next week is fine. Thank you!

@willkg
Copy link
Contributor

willkg commented Nov 21, 2017

@jayvdb It's been two weeks... how goes it? Is there anything I can do to help?

@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 23, 2017

I'm putting it together now. Should be a few hours max

@jayvdb
Copy link
Contributor Author

jayvdb commented Nov 23, 2017

Most of the goodness is now in #364

@willkg
Copy link
Contributor

willkg commented Nov 29, 2017

Given the last comment, I'm going to close this out.

@willkg willkg closed this Nov 29, 2017
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.

4 participants