-
Notifications
You must be signed in to change notification settings - Fork 197
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
added reasons why certain warnings were filtered in 'pytest.ini' #2431
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rahulsindhu01!
I think the correct issue this PR is referencing is #1805 -- if that's correct, do you mind updating your PR description?
The filtered warnings in the catalog already had some brief comments explaining them here, above the ones you added -- although I appreciate your added detail. For #1805 we'll want to add these comments to the API and ingestion server pytest.ini
files.
@stacimc |
@stacimc ive done the changes as you said me to do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @rahulsindhu01!
I left a few nitpicks about formatting/etc, but my blocking concern is that while these notes do a great job of explaining what each filtered warning is, I think they don't quite capture why they're considered safe to ignore (beyond wanting to declutter test logs).
I realize this is probably a difficult ask without having a lot of historical context on the project, so I took a look and left a few comments. @obulat may also have some good insight here!
@@ -5,5 +5,21 @@ pythonpath = . | |||
|
|||
filterwarnings= | |||
ignore:Unverified HTTPS request | |||
# Ignore warnings related to unverified HTTPS requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually sure this suppressed warning is still needed, maybe @dhruvkb can comment since it looks like he added it initially? There's also this somewhat related issue.
Ok @stacimc, what should i do now |
Hello @rahulsindhu01, I think these are the next steps needed for this PR:
Let us know if you have any questions, or if you'd like a maintainer to take over. |
@stacimc ill do it |
@stacimc ive rebased the file and made other changes that you requested aswell |
@rahulsindhu01 Thank you! Your changes look great, but it looks like there's been an issue with the rebase resulting in all of the additional changes. Since you're on a forked repo, did you first rebase Since the changeset is so small, if you have any more issues cleaning up the history it might even be easier to just open a new PR with just the final changes. Thanks so much for your patience on this one! |
Based on the low urgency of this PR, the following reviewers are being gently reminded to review this PR: @obulat Excluding weekend1 days, this PR was ready for review 15 day(s) ago. PRs labelled with low urgency are expected to be reviewed within 5 weekday(s)2. @rahulsindhu01, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
@rahulsindhu01 I'm going to draft this PR for the time being so our bot is placated, but please let us know if you need any assistance with Staci's last request 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the comments in api/pytest.ini
above the errors they refer to, as suggested by @stacimc .
In the future, do not open PRs from the main branch of your fork, @rahulsindhu01 : it would be impossible to create other PRs or update your main branch. Always create a new branch for a new PR.
Thank you for your contribution! 🌟
@@ -4,5 +4,12 @@ DJANGO_SETTINGS_MODULE = conf.settings | |||
pythonpath = . | |||
|
|||
filterwarnings= | |||
# Ignore warnings related to unverified HTTPS requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhruvkb , could you please take a look at this PR? Do we still need to suppress this warning?
Ok got it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution @rahulsindhu01
Changes are sufficient for the time being and mostly address the feedback request. If additional changes are needed, we can open new issues to explore specific warnings with staff-only labels. It's clear this particular issue requires historical and domain knowledge beyond the scope of what we usually ask for from new contributors.
Fixes
Fixes #1805 by @AetherUnbound
Description
Testing Instructions
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin