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.

Sign up for GitHub

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

Jump to bottom

Setup basic test suite #27

Merged
merged 33 commits into from
Oct 27, 2024
Merged

Setup basic test suite #27

merged 33 commits into from
Oct 27, 2024

Conversation

Stormheg
Copy link
Member

@Stormheg Stormheg commented Oct 25, 2024 •
edited
Loading

  • GitHub Actions
  • Pytest
  • Factories
  • Test models, utilities etc.
    • testing the actual views for registering and verifying is a bigger piece of work I haven't started yet
  • Fix a couple of bugs and undesirable behavior encountered along the way
  • Nightly testing to make sure we know what upcoming changes in upstream packages will affect us

todos:

  • Update DEVELOPMENT.md
  • Do PyPI publishing automation configuration

Stormheg added 21 commits October 25, 2024 17:05
@Stormheg
Add testing dependencies
d0ece00
@Stormheg
Add factories and pytest fixtures
f7c6c32
@Stormheg
Add model unit tests
8f0b9f5
@Stormheg
Remove unsupported key algorithms option from authenticate_complete call
d5b687c
@Stormheg
Make deferring supported algorithms config value to None...
97e95df 
...instead of magic `all` string value.
@Stormheg
Add transports factory data
d8a650b
@Stormheg
Add user_in_memory fixture
6b63f31 
Not performing database calls speeds up tests, so lets use an in-memory
user if we can.
@Stormheg
Move get_allowed_origins method up
eb1cd6e
@Stormheg
Add helper test cases
998f204
@Stormheg
Test settings
5c2729a
@Stormheg
Improve checks
a72e80f
@Stormheg
Test checks
c420b6e
@Stormheg
Test utils
78e46b0
@Stormheg
Do not auto-register model admin
2116808
@Stormheg
Add admin tests
af1b8fe
@Stormheg
Add a testapp that implements custom models
e291780
@Stormheg
Fix error on Python < 3.12
117fc04 
Fix unsupported operand error for older Pythons
@Stormheg
Add tox configuration
3af0c92
@Stormheg
Add codecov.yaml config
d5557a6
@Stormheg
Setup GitHub actions
ea39c00
@Stormheg
Ignore migrations and tests in coverage
7e5525d
@codecov Codecov
Copy link

codecov bot commented Oct 25, 2024

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

Stormheg
Stormheg commented Oct 25, 2024
View reviewed changes
.github/report_nightly_build_failure.py

import requests

if "SLACK_WEBHOOK_URL" in os.environ:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is an internal Slack channel. Not very visible... will probably re-asses at some point.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not against this. Integrations with common third party stuff is weird for sure. I've got some work related projects which have things like this for them, but also intend to release versions for other people. I've looked at several open source projects (both on purpose for this sort of thing and for other stuff where I just happened to notice this kind of integration) and it's not uncommon or unreasonable to have stuff like this as settings.

It definitely feels kind of weird to me to specifically support some things and not others even if they fill the same niche, but from what I've seen, it's not uncommon to support the ones you use.

Copy link
Member Author

Choose a reason for hiding this comment

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

For sure, that's why this is good enough for now. I'm just considering that if failures were more visible, it would attract (new) contributors to help fix and prepare for whatever upstream changes we need to adapt to.

Stormheg
Stormheg commented Oct 25, 2024
View reviewed changes
src/django_otp_webauthn/admin.py
Comment on lines -93 to -94
if credential_model == "django_otp_webauthn.WebAuthnCredential":
admin.site.register(WebAuthnCredential, WebAuthnCredentialAdmin)
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided I don't like this auto-registering behavior

@Stormheg Stormheg marked this pull request as draft October 25, 2024 15:35
@jmichalicek
Copy link
Contributor

Looking forward to finding a bit of free time to play with this.

@Stormheg Stormheg marked this pull request as ready for review October 27, 2024 17:41
@Stormheg
Remove as_credential_descriptors WebAuthnCredentialManager method
b64f3e2 
Should be using the QuerySet `as_credential_descriptors` method instead.
@Stormheg
Copy link
Member Author

@jmichalicek I'm going to merge and release this now. Feel free to review this if you like though! Happy to hear your thoughts.

@Stormheg Stormheg merged commit c1a93e5 into Stormbase:main Oct 27, 2024
9 checks passed
@Stormheg Stormheg deleted the tests branch October 27, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmichalicek jmichalicek jmichalicek left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Stormheg @jmichalicek