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

Database improperly closed in the test case within this tutorial #676

Merged
merged 1 commit into from May 14, 2013
Merged

Database improperly closed in the test case within this tutorial #676

merged 1 commit into from May 14, 2013

Conversation

adityaathalye
Copy link

os.unlink(flaskr.DATABASE) causes the actual application database to be
purged; whereas, I reckon, one wants the test database to be removed.
So every time I ran it, the test passed, but ended up crashing the live
app for want of a valid database.

I avoided using the sample code in examples/flaskr thus far, as I chose
to type out code from the turorial docs. The actual example code looks
good - at least to my beginner's eye.

os.unlink(flaskr.DATABASE) causes the actual application database to be
purged; whereas, I reckon, one wants the _test_ database to be removed.
So every time I ran it, the test passed, but ended up crashing the live
app for want of a valid database.

I avoided using the sample code in examples/flaskr thus far, as I chose
to type out code from the turorial docs. The actual example code looks
good - at least to my beginner's eye.
@adityaathalye
Copy link
Author

Hi, I'm wondering if I'm mistaken to suggest this edit - it says under the pull request, "The Travis build failed".

The build status report says 'Example code leaked' with reference to test_memory_consumption (flask.testsuite.regression.MemoryTestCase).

Did my change, to the tutorial text, cause that test to fail? If yes, can someone help me understand why?

I'll appreciate any feedback.
Thanks!

@adityaathalye
Copy link
Author

Thanks, Markus. I did notice failing tests attached to other pull requests. I was wondering if that was what cause this pull request to remain open.

I'll be glad to learn if my reasoning for the original commit is flawed - one more chance to improve :)

Thanks again for replying.

@untitaker
Copy link
Contributor

Whoops, accidentally deleted my comment. What i wanted to say is that this test case always seems to fail, and a fix in the docs should not trigger a testcase failure anyway.

@adityaathalye
Copy link
Author

Ah, so the culprit is elsewhere. Thanks again for the help.

@adityaathalye
Copy link
Author

According to this code, the regression test fails here for all Python other than CPython 2.7.

Don't understand why this part of the regression test breaks though.

Any ideas?

@untitaker
Copy link
Contributor

Running gc.collect() before counting the objects the second time seems to "fix" (?) the test.

@adityaathalye
Copy link
Author

Ok, so I tried to print len(gc.get_objects()) after _ _ enter _ _ and then after _ _ exit _ _. It returns a different number of objects - screenshots below. I'm using Python 2.7.3. So the _NoLeakAsserter fails rightfully for Python 2.7.

So now the question-- if we want to answer it --could be, why, according to this test, does flask encounter memory leaks, within a Python 2.7 environment, but not in 2.5, 2.6, and pypy?

regression-py_debugging_code_screenshot

regression-py_debugging_output_screenshot

@untitaker
Copy link
Contributor

The tests are not executed anywhere else than on Python 2.7.

@adityaathalye
Copy link
Author

@untitaker
Copy link
Contributor

I mean this particular testcase only asserts when on CPython 2.7. You can see that in the source.
-- Markus

@untitaker
Copy link
Contributor

@adityaathalye
Copy link
Author

Ah! I didn’t interpret that part of the code correctly before. Now I see how. Thanks, Markus.

From: Markus Unterwaditzer [mailto:notifications@github.com]
Sent: 21 February 2013 16:07
To: mitsuhiko/flask
Cc: Aditya Athalye
Subject: Re: [flask] Database improperly closed in the test case within this tutorial (#676)

https://github.com/mitsuhiko/flask/blob/master/flask/testsuite/regression.py#L77 to be precise.

Reply to this email directly or view it on GitHub #676 (comment) .
Image removed by sender.

mitsuhiko added a commit that referenced this pull request May 14, 2013
Database improperly closed in the test case within this tutorial
@mitsuhiko mitsuhiko merged commit 7265bb7 into pallets:master May 14, 2013
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants