Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Compress submissions on the client/decompress on the server #4

Merged
merged 7 commits into from Mar 30, 2012

Conversation

Projects
None yet
2 participants
Contributor

axitkhurana commented Mar 23, 2012

I have tried to take care of your suggestions. Please review the changes.

Owner

mapleoin commented Mar 24, 2012

The code looks good. Can we have some tests with this, please?

axitkhurana added some commits Mar 21, 2012

fix content type and encoding headers
Gzipped submission should have headers
 - Content-Type: text/plain
 - Content-Encoding: gzip
Compression enabled by default
take submission from received file if no gzip header
- If Content-Encoding: gzip not present in request, consider
received file as plaintext.
- Use with statement to automatically close gzipped submission
file descriptor
add tests for gzipped or plaintext submissions
tests for:
- gzipped file with Content-Encoding:gzip header
- plaintext file with Content-Encoding:gzip header
- plaintext submission without Content-Encoding:gzip header

Why aren't these three separate tests? They all seem to be independent.

Owner

axitkhurana replied Mar 29, 2012

I was trying 1 test per view, but separating them makes much more sense, thanks.

A good general rule is to have one assertion per test. That's sometimes hard to achieve, but not in this case. One assertion per test makes things a lot easier to debug later.

This code looks like it belongs in the tearDown method (except for the init_db call). Then it gets called after each test automatically and you don't need to call reset_db manually.

Urgh, looking more at the code, I don't think you need this code at all, db_session.remove() in tearDown destroys the database since it's only in-memory.

Owner

axitkhurana replied Mar 29, 2012

Thanks, I could use have used a temporary file[1], which is better in-memory or temporary file?

[1] http://flask.pocoo.org/docs/testing/#the-testing-skeleton

I'm not sure of all the advantages and disadvantages of one method over the other. I suppose the in-memory database would be much faster than the tempfile one, while the tempfile might provide a way to inspect the database manually after the tests are run. I suppose either is fine. Maybe we should keep it the same with the other test modules though and eventually move the database init for tests in a central place.

I'm just being overly picky here, but you don't need all these Arches for the submission-text in the fixtures. Although, I guess you can leave it as it is for now and we'll add a fixture for pre-populating the database for tests later.

maybe you should be using assertIn[1] instead of assert here, since you're importing (and not currently using AFAICS) unittest2 anyway.

[1] http://docs.python.org/library/unittest.html#unittest.TestCase.assertIn

these comments are superfluous, since we already have a way to see which are test methods and which aren't: we give them a name that starts with "test_"

this isn't needed, since we have tools which automatically discover tests (nosetests, unittest discover in py2.7 etc.)

where is f_in closed if you go on this branch?

Owner

axitkhurana replied Mar 29, 2012

f_in (same as f in this branch) is passed to view that reads the file, shouldn't it remain open? I could make this code more explicit[1]. I think, in the receive submission view, we need opened file to perform I/O operation(read). Also, we don't close the received file in views, do we? [2]

[1] http://paste.pocoo.org/show/573236/
[2] http://flask.pocoo.org/docs/patterns/fileuploads/

err... yes. I think you're right, I didn't read the code properly.

Can you add a license txt to this file with your name and email? You can copy it from one of the other files.

axitkhurana added some commits Mar 30, 2012

separate submission tests, remove reset_db
- separate the three independent tests for submission
- reset_db is not required as db_session.remove() in tearDown
destroys the database since it's only in-memory.
Contributor

axitkhurana commented Mar 30, 2012

Thanks Ionuț for the comments, I've incorporated your suggestions. Can you please have a look?

Owner

mapleoin commented Mar 30, 2012

Looks good, can you also remove the "helper functions" "testing functions" comments?

Contributor

axitkhurana commented Mar 30, 2012

Removed.

Owner

mapleoin commented Mar 30, 2012

Awesome! Thanks a lot, Akshit! Pull request accepted.

mapleoin added a commit that referenced this pull request Mar 30, 2012

Merge pull request #4 from axitkhurana/compress-submissions
Compress submissions on the client/decompress on the server

@mapleoin mapleoin merged commit 9e7722d into mapleoin:master Mar 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment