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

Fix the test for basic app #221

Merged
merged 1 commit into from
Nov 21, 2023
Merged

Fix the test for basic app #221

merged 1 commit into from
Nov 21, 2023

Conversation

greyli
Copy link
Member

@greyli greyli commented Nov 17, 2023

  1. Update the test app

If I understand correctly, the debug toolbar will only be enabled if app.debug is True. So I added the DEBUG=True to the test app.

Related code:

'DEBUG_TB_ENABLED': app.debug,

  1. Update the src/flask_debugtoolbar/__init__.py

Fix the two if statements to prevent the following errors:

>       if 'gzip' in response.headers.get('Content-Encoding'):
E       TypeError: argument of type 'NoneType' is not iterable

Since the response.headers.get('Content-Encoding') could be None.

With this PR, all the tests will be passed. The failed style checker will be fixed in #219

Copy link
Contributor

@macnewbold macnewbold left a comment

Choose a reason for hiding this comment

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

This looks great, and looks like it's passing the automated tests other than stylecheck. If we can get that one passing, I'm 100% on board with merging this. If we can't and there's a good reason to merge it with that failing, I'm open minded to that too.

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Everything looks good to me except I don't understand the rationale for the removal of TESTING=True.

@@ -7,7 +7,6 @@

def load_app(name):
app = __import__(name).app
app.config['TESTING'] = True
Copy link
Member

@jeffwidman jeffwidman Nov 20, 2023

Choose a reason for hiding this comment

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

I thought it was best practice to set this when in tests?

Looks like that's still true for Flask 3.0.x: https://flask.palletsprojects.com/en/3.0.x/testing/

I realize the main side-effect of propagating exceptions also happens when DEBUG is True, but still seems best to follow best practices as later on additional side effects may be added.

Overall, the presence/absence of this seems like it shouldn't affect whether the tests pass/fail which is the purpose of this PR, so IMO I'd extract this change to a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review. I guess I made a mistake here. I don't need to remove this line, but just add the debug = True line. Updated.

@jeffwidman
Copy link
Member

I just merged #219 and rebased this, nice to see everything 🟢 !

Copy link
Member

@jeffwidman jeffwidman left a comment

Choose a reason for hiding this comment

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

Awesome thanks so much for this!

@jeffwidman jeffwidman merged commit e6ae9d0 into pallets-eco:master Nov 21, 2023
10 checks passed
@jeffwidman
Copy link
Member

Now that these checks are all passing, I went ahead and enabled them all as required checks against master.

I have zero objections if for some reason another maintainer needs to temporarily override these and merge a PR, but obviously use your judgement as that should probably be the exception rather than the rule now that folks have gone to so much effort to bring us up to date.

cc @macnewbold @nickjj @greyli

@greyli greyli deleted the fix-ut branch November 21, 2023 13:36
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants