Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

[INCR-22] Support testing Python 3 in edx-oauth2-provider #58

Merged
merged 8 commits into from
May 24, 2019

Conversation

jonkiparsky
Copy link
Contributor

JIRA link

Changes:

  • update tox.ini per changes by bbearce on his previous attempt at this work (First attempt at editing the tox.ini file #56)
  • modify target test in Makefile, which was attempting to run coverage as well as tests
  • bump some versions in test_requirements.txt to allow make test and make coverage to run
  • add some notes in README.txt

Note: make test-all runs with failures, as expected.

@openedx-webhooks
Copy link

Thanks for the pull request, @jonkiparsky! I've created OSPR-3450 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • edx-code email threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@natabene
Copy link

@jonkiparsky Thank you for your contribution. Please let me know once all tests are green.

@jonkiparsky
Copy link
Contributor Author

@jmbowman Sorry about the delay on this, but I think this is ready for you now (getting all and only the expected failures)

Copy link

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few suggestions for further improvement.

tox.ini Outdated
@@ -1,5 +1,5 @@
[tox]
envlist = django{18,110,111}
envlist = py{36,37}-django{111,20,21,22}

Choose a reason for hiding this comment

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

Should probably add py27 to this also.

tox.ini Outdated
django111: Django>=1.11,<2.0
django20: Django>=2.0,<2.1
django21: Django>=2.1,<2.2
django22: Django>=2.2

Choose a reason for hiding this comment

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

This should have an upper bound in preparation for when the next major version of Django is released.

- TOXENV=django111
- TOXENV=django20
- TOXENV=django21
- TOXENV=django22

Choose a reason for hiding this comment

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

The failing test workers should be flagged as ok to fail via allow_failures until they can be fixed: https://docs.travis-ci.com/user/customizing-the-build/#rows-that-are-allowed-to-fail

@jonkiparsky
Copy link
Contributor Author

@jmbowman Thanks for the feedback. Hope this is satisfactory.

@natabene
Copy link

@jmbowman This is ready for your review.

@edx edx deleted a comment from openedx-webhooks May 24, 2019
Copy link

@jmbowman jmbowman left a comment

Choose a reason for hiding this comment

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

Thanks for getting that sorted out! We're looking into a switch from Travis to GitLab CI, which actually allows local execution of configured tests. Oh, and obligatory XKCD: https://xkcd.com/1296/

@jmbowman jmbowman merged commit 2581d6a into edx:master May 24, 2019
@openedx-webhooks
Copy link

@jonkiparsky 🎉 Your pull request was merged!

Please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants