Skip to content

Fix tests#381

Merged
Natim merged 3 commits intojazzband:masterfrom
Atorich:fix_tests
Aug 20, 2019
Merged

Fix tests#381
Natim merged 3 commits intojazzband:masterfrom
Atorich:fix_tests

Conversation

@Atorich
Copy link
Copy Markdown
Contributor

@Atorich Atorich commented Aug 5, 2019 •
edited
Loading

Problem

I have found that AbstractModelTrackerTests is not quite correct.
Since it inherited from FieldTrackerTestCase that just implements some extra assertions, there are no actual tests for this case.

Solution

I have fixed AbstractModelTrackerTests and it seems that some tests are failed for the case with models inherited from some AbstractModel with fields defined (like AbstractUser and its children)

@Atorich Atorich mentioned this pull request Aug 5, 2019
Open
@Natim Natim closed this Aug 20, 2019
@Atorich
Copy link
Copy Markdown
Contributor Author

Atorich commented Aug 20, 2019

Just closed without any comments?

@Natim
Copy link
Copy Markdown

Natim commented Aug 20, 2019

The PR says Fix tests, but tests work on master and the CI status is a failing tests.
The PR is one line change and break the tests. I don't know what more to tell.

Can you elaborate on what you are trying to do?

@Atorich
Copy link
Copy Markdown
Contributor Author

Atorich commented Aug 20, 2019 •
edited
Loading

Have you read the description?
AbstractModelTrackerTests does not test anything (because there are no test methods in the FieldTrackerTestCase class) -- that's why it seems like "tests work".
https://github.com/Atorich/django-model-utils/blob/23148f481c4c9fedd69f68a85779178846fb352d/tests/test_fields/test_field_tracker.py#L16

One line change -- is something that actually runs the tests which are failing, you're right.
ModelTrackerTests (which I used instead of FieldTrackerTestCase) - the common class that contains test_ methods.

@Natim
Copy link
Copy Markdown

Natim commented Aug 20, 2019 •
edited
Loading

Ok this seems to be related to #80 #147
@Atorich can you think of how we could fix the tests?

@Natim Natim reopened this Aug 20, 2019
@Atorich
Copy link
Copy Markdown
Contributor Author

Atorich commented Aug 20, 2019 •
edited
Loading

There is nothing to fix in the tests.
The problem with using FieldTracker on models marked as abstract=True is still here and my PR just proves it.

I think it is better to keep the test case from my PR but @skip it for now because of further investigation here and here means that the problem is out of the scope of model-utils.
But leaving ModelTrackerTests as-is is still confusing.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 20, 2019 •
edited
Loading

Codecov Report

Merging #381 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #381      +/-   ##
==========================================
+ Coverage   91.72%   91.77%   +0.05%     
==========================================
  Files           6        6              
  Lines         761      766       +5     
==========================================
+ Hits          698      703       +5     
  Misses         63       63
Impacted Files Coverage Δ
model_utils/choices.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3d0881...7d9ebc1. Read the comment docs.

@Natim
Copy link
Copy Markdown

Natim commented Aug 20, 2019 via email

@Natim Natim merged commit d51368c into jazzband:master Aug 20, 2019
cr313 added a commit to cr313/model-utils that referenced this pull request Apr 19, 2024
@cr313
Skip AbstractModelTrackerTests
ddb2cc7 
jazzband/django-model-utils#381
mthuurne added a commit to ProtixIT/django-model-utils that referenced this pull request Jun 12, 2024
@mthuurne
Remove catching of AttributeError in DescriptorWrapper
14cf1b0 
This catch was introduced in jazzband#367, but subsequent discussion
in jazzband#370, jazzband#381 and jazzband#382 suggests that it wasn't the proper solution.

Now that `AbstractModelTrackerTests` is re-enabled and passing,
it becomes clear that `AttributeError` is no longer raised during
unit testing when using a recent Django version. Therefore I think
it is safe to remove the `try`/`except`.
@mthuurne mthuurne mentioned this pull request Jun 12, 2024
Merged
5 tasks
mthuurne added a commit to ProtixIT/django-model-utils that referenced this pull request Jun 13, 2024
@mthuurne
Remove catching of AttributeError in DescriptorWrapper
23f1764 
This catch was introduced in jazzband#367, but subsequent discussion
in jazzband#370, jazzband#381 and jazzband#382 suggests that it wasn't the proper solution.

Now that `AbstractModelTrackerTests` is re-enabled and passing,
it becomes clear that `AttributeError` is no longer raised during
unit testing when using a recent Django version. Therefore I think
it is safe to remove the `try`/`except`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Atorich @Natim