Skip to content

Conversation

@n2ygk
Copy link
Contributor

Fixes#1065

Description of the Change

  • Flags no coverage for cleartoken management command since the test is in test_models.
  • Expand coverage of the clear_expired tests.

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

@n2ygkn2ygk marked this pull request as draft January 11, 2022 21:56
@codecov
Copy link

codecovbot commented Jan 11, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.87%. Comparing base (689269e) to head (40dba48).
Report is 203 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #1088 +/- ## ========================================== + Coverage 96.65% 96.87% +0.21%  ========================================== Files 31 31 Lines 1763 1759 -4 ========================================== Hits 1704 1704 + Misses 59 55 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@n2ygk
Copy link
ContributorAuthor

@Andrew-Chen-Wang See 6ddb3d7 for broken commit due to: ValueError: bulk_create() prohibited to prevent data loss due to unsaved related object 'access_token'. in attempting to follow your advice at #1084 (comment)

@n2ygkn2ygk added this to the 1.7.0 milestone Jan 11, 2022
Copy link
Contributor

@Andrew-Chen-WangAndrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

f-strings used for performance and readability improvement.

bulk_create does not call save(), but the pks are set given the database is Postgres, MariaDB, or SQLite3 (at some version+ that GitHub actions and all modern computers prob already have).

For reference, here's where it's set: https://github.com/django/django/blob/dc9deea8e85641695e489e43ed5d5638134c15c7/django/db/models/query.py#L514

@n2ygk
Copy link
ContributorAuthor

I'll circle back soon and make sure my test logic is correct (probably isn't) now that the code is running;-)

@n2ygkn2ygk marked this pull request as ready for review January 12, 2022 22:37
Copy link
Contributor

@Andrew-Chen-WangAndrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

LGTM!

@n2ygkn2ygk merged commit ac20152 into django-oauth:masterJan 12, 2022
@n2ygkn2ygk deleted the test_models_cleartokens branch January 12, 2022 23:53
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Write a test for cleartokens management command

2 participants

@n2ygk@Andrew-Chen-Wang