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

Ensure current unit tests pass on CI under Python 2.7 #23

Closed
1 of 3 tasks
hornc opened this issue Aug 14, 2019 · 6 comments
Closed
1 of 3 tasks

Ensure current unit tests pass on CI under Python 2.7 #23

hornc opened this issue Aug 14, 2019 · 6 comments

Comments

@hornc
Copy link
Collaborator

hornc commented Aug 14, 2019

Commit b8831c7 removed the existing tests from running on CI, which enabled a parse error to slip into master, involving a try with a missing except block which is fixed by #20

After some discussion (amongst archive.org staff and volunteers) we agreed the following:

Therefore we need to upgrade this internetarchive/infogami fork to Python 3 to support Open Library.

To do this safely since this codebase has had had much activity for a long time, we need to have confidence that we are not breaking anything in our fork with current changes.

  • Get existing unit tests running and passing on CI under Python2.7 to provide a baseline for our fork (run under Python 3 for information only, in allowed failures)
  • Get flake8 tests passing under Python 2.7
  • Focus on fixing Python 3 failures, move this out of allowed failures
@cclauss
Copy link

cclauss commented Aug 14, 2019

My apologies for b8831c7 and thanks for you efforts to repair it. The plan above sounds most resonable to me.

@hornc
Copy link
Collaborator Author

hornc commented Aug 19, 2019

I have a branch https://github.com/internetarchive/infogami/tree/repair-tests where the Travis CI results match the test results I get locally at this commit
85ff436

Results: https://travis-ci.org/internetarchive/infogami/jobs/573626882

Which is failing on the issue reported at infogami#4

I thought I'd fixed it in #9 , but the 'unused' import I removed is used in that file, so it's not a good fix.

To add the suggested app = web.application(urls, globals(), autoreload=False) fix, we need to provide something sensible for urls in delegate.py, I'm not immediately sure what that should be.

@cclauss
Copy link

cclauss commented Aug 19, 2019

For now, can we put a # noqa linter directive on the import line so that we can progress beyond that one?

@hornc
Copy link
Collaborator Author

hornc commented Aug 21, 2019

The app issue that caused tests to fail was resolved in 7ce3879 by changing the order of importing delegate

@cclauss
Copy link

cclauss commented Aug 22, 2019

Can this be closed now?

@hornc
Copy link
Collaborator Author

hornc commented Aug 22, 2019

yes, Closing as done, Python 2.7 tests are passing.

@hornc hornc closed this as completed Aug 22, 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

No branches or pull requests

2 participants