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

Fix tests #1198

Closed
wants to merge 4 commits into from
Closed

Fix tests #1198

wants to merge 4 commits into from

Conversation

corubba
Copy link
Contributor

@corubba corubba commented May 18, 2022

The tests were in a sad state, and still are. But at least they run and pass again.

Fix test docker

The Dockerfile did not work as is, because the dependencies in requirements.txt are newer than the stretch-image with its python v3.5 can support/run. Use stable debian with the lts nodejs instead, plus had to add some libs to make the wheel build succeed.
jsonschema v4 breaks things, so its version needs to be pinned until bravado is fixed.

Fix tests

Increased the version of pytest to make it work with py 3.10.
The GET calls no longer return list but the object itself, fixed the tests and assertions to account for that. The tests did not account for the later added allow_user_remove_domain setting. And there were issues with missing and non-stopped patchers/mocks.

@jbe-dw jbe-dw added this to To do in Pull Requests Tracking May 20, 2022
@jbe-dw jbe-dw added this to the v0.4.0 milestone Jun 17, 2022
@jbe-dw
Copy link
Contributor

jbe-dw commented Jun 17, 2022

Hello,

To be transparent we are not great at test now. It is something that has to be improved all the way though. We keep it aside now (don't worry if not merged quickly).

I've set up milestones about some opened PR that may need to be included in the test sequence as well.

Please let us know your feelings about this, if there's not much extra work for you to check and add them. I understand if it's not in your priorities/perimeter.

Regards

@corubba
Copy link
Contributor Author

corubba commented Jun 18, 2022

This PR is only fixing exiting tests, to hopefully spare others the time spent searching why they are failing only to realise that it's not their local dev-env's fault but the tests are simply broken to begin with.
In the end it is the maintainers call in what form to request or even require tests and test coverage for changes. Imho fixing the tests after any change in behaviour and adding new tests for new features should be part of the respective PR, and done by the respective authors.

That being said I just rebased this onto master, and while the the tests still run some are now failing because #1089 broke stuff; namely the exceptions on DELETE caused by None that were mentioned.

@jbe-dw
Copy link
Contributor

jbe-dw commented Jun 18, 2022

I agree with you, but the project is not yet prepared for such things. Tests should be here to ensure that nothing breaks. But as you could probably see there is no rule about formatting or testing, or even cleanly providing good git PR.

We will probably focus on this aspect at some point. I'll try the test soon and propose a corrective PR afterwards.

Thanks.

@corubba
Copy link
Contributor Author

corubba commented Jun 23, 2022

Now all tests pass again.

The Dockerfile did not work as is, because the dependencies in
requirements.txt are newer than the stretch-image with its python v3.5
can support/run. Use stable debian with the lts nodejs instead, plus
had to add some libs to make the wheel build succeed.
jsonschema v4 breaks things, so its version needs to be pinned until
bravado is fixed [0].

[0] https://github.com/Yelp/bravado-core/pull/385/files#r731674447
Increased the version of pytest to make it work with py 3.10 [0].
The GET calls no longer return list but the object itself, fixed the
tests and assertions to account for that. The tests did not account for
the later added `allow_user_remove_domain` setting. And there were
issues with missing and non-stopped patchers/mocks.

Now all tests are at least passing.

[0] pytest-dev/pytest#8540
PR #1089 is the culprit, as was already predicted in the review.
The `unit/apikey` directory is removed because it does not contain any
tests. Same for `unit/test_decorators.py`.
The `fixture` module is renamed to the special-name `conftest` [0] so
they are available in all tests without the need to import them. With
that in place, I removed all now unneeded or previously already unused
imports from the tests.
Also removed that wierd `sys.path` bit from `unit/zone/test_admin_apikey.py`,
no idea what that was originally intended for.

[0] https://docs.pytest.org/en/6.2.x/fixture.html#conftest-py-sharing-fixtures-across-multiple-files
@corubba
Copy link
Contributor Author

corubba commented Jun 24, 2022

Rebased onto master, and added a new commit to cleanup a few things.

Cleanup

The unit/apikey directory is removed because it does not contain any tests. Same for unit/test_decorators.py.
The fixture module is renamed to the special-name conftest so they are available in all tests without the need to import them. With that in place, I removed all now unneeded or previously already unused imports from the tests.
Also removed that weird sys.path bit from unit/zone/test_admin_apikey.py, no idea what that was originally intended for.

@ymage ymage mentioned this pull request Dec 22, 2022
@AzorianMatt
Copy link
Member

This PR is superseded by PR 1339 so I'm closing it.

Pull Requests Tracking automation moved this from To do to Done Feb 17, 2023
@corubba corubba deleted the bugfix/tests branch March 1, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants