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
feat: [WIP] Migration to nox
testing automation framework
#466
Conversation
The latest test log:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of unrelated changes for one PR, and it looks to me like copying the noxfile straight from python-spanner will cause more harm than good here.
Since the formatting changes are mixed in with code and config changes, this will make for a difficult git history too. Better to quarantine the formatting changes in their own PR, maybe at the same time as the change to the linter/formatter config that makes these changes required.
I think we'll save a lot of time by starting with working config and building up to the same (or similar) set of targets as in python-spanner instead of checking in a broken config and then working backwards.
Here's what I'd recommend:
- Switch from tox to nox, with the same set of targets as in
tox.ini
now - Add the
docs
target and make the changes required to get the docs to build - Add test targets for other versions of python and django, these are sure to fail right now without any test changes
- Add the
lint
target and make automated formatting changes viablack
- Add the coverage target
- Add system/integration tests, then add the
system
target
Also, why this change?
tests/spanner_dbapi
-->tests/unit
Splitting the tests by module makes sense to me, even if spanner_dbapi
is the only module with tests at the moment. Also all tests so far are unit tests, I don't see the argument for a separate tests/unit/
dir.
And this one:
django_spanner
-->google/cloud/spanner_django
Should this be in the google.cloud
namespace package? Maybe it should! But I don't know that the change should be part of this PR.
Looks like we can close this now that #480 is in. |
Since
nox
is the primary testing automation tool used by Google API, we believe that its earliest integration with the Spanner-Django project is necessary to ensure its continuous compliance with the coding standards from the start. This is also a step towards future merging of Django-related code into the main Spanner repository and may save considerable amount of time and effort at later stages of development.Major change list:
noxfile.py
[largely copied from Spanner main repo];docs
google
google/cloud
docs/_static
django_spanner
-->google/cloud/spanner_django
;spanner_dbapi
-->google/cloud/spanner_dbapi
;tests/spanner_dbapi
-->tests/unit
;examples
-->samples
;__init__.py
andconf.py
files;custom.css
[copied from main Spanner repo];setup.py
;README.md
converted toREADME.rst
[fixing some formattercheck
errors];docs/index.rst
file to comply with Sphinx requirements (the file is blank and to be updated as necessary);.gitignore
.From the results of the initial testing, the code has numerous assertion errors and coverage gaps, see the test log below, which have little to do with the test setup as such. Therefore, addressing those will be a subject of subsequent PRs, if and when the matters of the current proposition are reviewed and agreed upon.
Towards #474.