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

Task/wp 209 fix deprecated warnings (part II) #852

Merged
merged 10 commits into from
Sep 26, 2023

Conversation

van-go
Copy link
Contributor

@van-go van-go commented Aug 24, 2023

Overview

This is a continuation from WP-171.
When running unit tests, a lot of deprecated warnings were appearing. This tickets addresses those warnings.

Related

Changes

  • The previous ticket [WP-171] addressed issues with our code (Core-Portal). However, a lot of the warnings being seen are issues with Django. To keep these from showing up, they are filtered out; pytest.ini now has 'filterwanrings'.

  • PytestUnknownMarkWarning: Unknown pytest.mark.asyncio was addressed by adding markers = asyncio: mark a test as asyncio to pytest.ini

  • RemovedInDjango40Warning: The providing_args argument is deprecated. was addressed in signals.py by redefining portal_event as suggested in the Django documentation.

Testing

  1. Start docker container —> docker exec -it core_portal_django bash
  2. Run pytest -ra portal/apps/auth/api/
  3. Notice there are no more deprecated warnings.

UI

Screen Shot 2023-08-24 at 12 32 07 PM

Notes

Running pytest -ra portal/apps/ shows that some tests are failing, and also has 7 warnings (not deprecation warnings). These might need to be addressed.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #852 (f034d9d) into main (574a567) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #852   +/-   ##
=======================================
  Coverage   63.05%   63.05%           
=======================================
  Files         427      427           
  Lines       12144    12144           
  Branches     2492     2492           
=======================================
  Hits         7658     7658           
  Misses       4281     4281           
  Partials      205      205           
Flag Coverage Δ
javascript 68.88% <ø> (ø)
unittests 57.27% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
server/portal/apps/signals/signals.py 100.00% <100.00%> (ø)

Copy link
Collaborator

@chandra-tacc chandra-tacc left a comment

Choose a reason for hiding this comment

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

LTGM.
Just a note: The test ran only on this folder portal/apps/auth/api and there are no warnings.
In next PR, may be expand this to all portal/apps.

server/portal/apps/signals/signals.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@chandra-tacc chandra-tacc left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@shayanaijaz shayanaijaz left a comment

Choose a reason for hiding this comment

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

LGTM

ignore:.*Django now detects this configuration.*:django.utils.deprecation.RemovedInDjango41Warning

markers =
asyncio: mark a test as asyncio.
Copy link
Member

@rstijerina rstijerina Sep 11, 2023

Choose a reason for hiding this comment

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

Newline at end of file

Suggested change
asyncio: mark a test as asyncio.
asyncio: mark a test as asyncio.

server/portal/apps/signals/signals.py Outdated Show resolved Hide resolved
Comment on lines +6 to +12
ignore::DeprecationWarning:django.*:
ignore::DeprecationWarning:openapi_schema_validator.*:
ignore::DeprecationWarning:openapi_spec_validator.*:
ignore::DeprecationWarning:openapi_core.*:
ignore::DeprecationWarning:kombu.*:
ignore::DeprecationWarning:pkg_resources.*:
ignore:.*Django now detects this configuration.*:django.utils.deprecation.RemovedInDjango41Warning
Copy link
Member

Choose a reason for hiding this comment

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

Do we want these warnings removed? I think it might be better if we resolved the deprecation warnings, as in changed code to resolve the warnings, rather than suppressed the warnings.

Copy link
Contributor Author

@van-go van-go Sep 11, 2023

Choose a reason for hiding this comment

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

This would require updating the Django version. There aren't any changes in our code to make that would resolve these warnings. Do you know when we are planning to update the Django version? We can add a note or ticket to review these warnings.

Copy link
Collaborator

@chandra-tacc chandra-tacc Sep 12, 2023

Choose a reason for hiding this comment

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

@van-go , just so that we do not ignore this permanently, would it be possible to add tracking jira tasks and add the task number to the ini file (comments start with ';' in ini file, afaik).

  • django version fix: 4.2.5
  • openapi:
    openapi_spec_validator 0.5.0
    openapi_schema_validator 0.6.0
    openapi_core 0.18
  • kombu: 5.3.2.
  • which third party throws : django.utils.deprecation.RemovedInDjango41Warning
    Move channels to 4.0.0.

The edits for these will be in dependency list

Adding a note in task to remove the ignore when the task is fixed will ensure that the ignore is not left behind permanently.

@chandra-tacc chandra-tacc merged commit 9caf6a8 into main Sep 26, 2023
6 checks passed
@chandra-tacc chandra-tacc deleted the task/wp-209-fix-deprecated-warnings-II branch September 26, 2023 18:45
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.

4 participants