Skip to content

Conversation

n2ygk
Copy link
Contributor

@n2ygk n2ygk commented Jan 11, 2022

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

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

codecov bot 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
Contributor Author

n2ygk commented Jan 11, 2022

@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)

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

@Andrew-Chen-Wang Andrew-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 and others added 3 commits January 12, 2022 08:46
Co-authored-by: Andrew Chen Wang <60190294+Andrew-Chen-Wang@users.noreply.github.com>
@n2ygk
Copy link
Contributor Author

n2ygk commented Jan 12, 2022

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

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

@Andrew-Chen-Wang Andrew-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!

@n2ygk n2ygk merged commit ac20152 into django-oauth:master Jan 12, 2022
@n2ygk n2ygk deleted the test_models_cleartokens branch January 12, 2022 23:53
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

Successfully merging this pull request may close these issues.

Write a test for cleartokens management command
2 participants