-
-
Notifications
You must be signed in to change notification settings - Fork 147
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
Conversation
d8ebfc9
to
88816bd
Compare
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.
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.
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.
Everything looks good to me except I don't understand the rationale for the removal of TESTING=True
.
test/test_toolbar.py
Outdated
@@ -7,7 +7,6 @@ | |||
|
|||
def load_app(name): | |||
app = __import__(name).app | |||
app.config['TESTING'] = True |
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 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.
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 review. I guess I made a mistake here. I don't need to remove this line, but just add the debug = True
line. Updated.
I just merged #219 and rebased this, nice to see everything 🟢 ! |
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.
Awesome thanks so much for this!
Now that these checks are all passing, I went ahead and enabled them all as required checks against 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. |
If I understand correctly, the debug toolbar will only be enabled if
app.debug
isTrue
. So I added theDEBUG=True
to the test app.Related code:
flask-debugtoolbar/src/flask_debugtoolbar/__init__.py
Line 114 in 2b8bf9c
src/flask_debugtoolbar/__init__.py
Fix the two if statements to prevent the following errors:
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