citusdata / django-multitenant

Python/Django support for distributed multi-tenant databases like Postgres+Citus
MIT License
708 stars 116 forks source link

Adds tests for apps.get_model #152

Closed gurkanindibay closed 1 year ago

gurkanindibay commented 1 year ago

In issues #99 and #138, we had some problems when getting tenant column, when getting class with apps.get_model Problem fixed wirh previous developments. I added a test to test the case

codecov[bot] commented 1 year ago

Codecov Report

Merging #152 (469f3d6) into main (d57f86f) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #152      +/-   ##
==========================================
+ Coverage   98.01%   98.03%   +0.02%     
==========================================
  Files          32       33       +1     
  Lines        1006     1020      +14     
==========================================
+ Hits          986     1000      +14     
  Misses         20       20              
Impacted Files Coverage Δ
...ngo_multitenant/tests/migrations/0024_data_load.py 100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

agedemenli commented 1 year ago

there's a skipped test in the check details. As discussed offline, we'll create another issue for it. It's unrelated to this pr

gurkanindibay commented 1 year ago

Two questions:

  • The migration reverse_func() is uncovered by tests. While that seems potentially harmless, is it typical for the other test migrations? Or do we cover them by tests in some way?
  • You're adding the test after the fix. Have you verified that the test fails prior to applying the fix? (Just want to be sure it's an effective test.)

Removed reverse_func, Yes always verifying. When I'm working tests always fails, at the end I succeed to make them pass