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

Removing relation child while updating nullable fk as None #1230

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

amit12297
Copy link

@amit12297 amit12297 commented Nov 24, 2023

Fixes #1229

Removing relation child while updating nullable fk as None in descriptors.py->RelationDescriptor

Ran the test cases locally using pytest -svv --cov=ormar --cov=tests --cov-fail-under=100 --cov-report=term-missing

All test cases passed

Screenshot 2023-11-25 at 2 00 56 AM

@codecov-commenter
Copy link

codecov-commenter commented Nov 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3a206dd) 100.00% compared to head (57ab550) 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1230   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          201       201           
  Lines        16647     16659   +12     
=========================================
+ Hits         16647     16659   +12     
Files Coverage Δ
ormar/models/descriptors/descriptors.py 100.00% <100.00%> (ø)
tests/test_relations/test_foreign_keys.py 100.00% <100.00%> (ø)

@collerek
Copy link
Owner

collerek commented Dec 6, 2023

Hi,

Can you add a test that fails now and that this issue fixes?
Can be based on #1229 (without fastapi part, just ormar code is enough).
Please place it under tests/test_relations/test_foreign_keys.py as an additional test case.

@amit12297
Copy link
Author

amit12297 commented Dec 6, 2023

Hi,

Can you add a test that fails now and that this issue fixes? Can be based on #1229 (without fastapi part, just ormar code is enough). Please place it under tests/test_relations/test_foreign_keys.py as an additional test case.

Hi @collerek,

I added the test-case. It fails without my changes and is passing with my changes. Please suggest if I can give a better name to the test-case.

Here are the screenshots-

Failing without my changes:
Screenshot 2023-12-07 at 1 08 33 AM
Screenshot 2023-12-07 at 1 21 18 AM

Passed with my changes:
Screenshot 2023-12-07 at 1 10 22 AM
Screenshot 2023-12-07 at 1 19 35 AM

@amit12297
Copy link
Author

@collerek please merge this if everything looks fine

Copy link

codspeed-hq bot commented Feb 12, 2024

CodSpeed Performance Report

Merging #1230 will degrade performances by 23.48%

Comparing amit12297:fix-fk-null-update (57ab550) with master (3a206dd)

Summary

❌ 1 regressions
✅ 83 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark master amit12297:fix-fk-null-update Change
test_get_or_none[250] 10 ms 13.1 ms -23.48%

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.

Nullable ForeignKey field does not get updated to null
3 participants