Remove warnings about unclosed file after running tests

[Joel-hanson]

This is a pull request related to issue #39 .

• What I have done is to prevent the stopping of web service without invoking closing the period database files. So I have made a keyword argument in create_app named return_server to return server variable thus we can stop the server after werkzeug.server.shutdown is called (done in _test/testcli.py)

• At last, the ValueError is added in the exception list (file financeager/httprequests.py) so that the JSONDecodeError is got (Since simplejson.decoder.JSONDecodeError actually inherits from ValueError - https://stackoverflow.com/questions/8381193/handle-json-decode-error-when-nothing-returned) . This is done because of the error in test test.test_cli.CliFlaskTestCase.test_communication_error.

[coveralls]

Coverage remained the same at 100.0% when pulling 9cae298f2763efb3def4655fa2759bc45f2783e6 on Joel-hanson:remove-resource-warning into 9876b0ec4272fe7ab28da74d2343b0e0fcd28bea on pylipp:master.

[pylipp]

Thanks you very much, this does the trick! I'm a little hesitant though about the change in fflask.create_app. I don't consider it clean style to extend production code for the sake of the test environment - do you think there is another way to get a reference to the used Server instance, maybe by digging into the registered resources of the app?

[Joel-hanson]

Yes, you are right will check that @pylipp .

[Joel-hanson]

Hi @pylipp, I have checked the other way of getting a Server variable but didn't find any can you please suggest any other solution for this.

[pylipp]

Without looking into the flask-restful source code, another way came to my mind (still violating the above princicle - but at least the signature and return value of create_app() does not change): we could assign app._server = server in create_app() and access the attribute in the test teardown?

[Joel-hanson]

Ok great I will do this @pylipp .

Thanks for the help.

[Joel-hanson]

Hi, @pylipp Have updated the code as per reviewed can you please check.

Thanks in advance

[pylipp]

Looks good! Can you explain why you added the ValueError catch? I don't see how having the explicit JSONDecodeError does not suffice.

[Joel-hanson]

Hi @pylipp , Since simplejson.decoder.JSONDecodeError actually inherits from ValueError - https://stackoverflow.com/questions/8381193/handle-json-decode-error-when-nothing-returned

This is done because of the error in test test.test_cli.CliFlaskTestCase.test_communication_error

[pylipp]

We're not using simplejson though?

[Joel-hanson]

Yes but when I debugged the 91st line in httprequests.py found this issue

That's why I made it be like this to catch the exception. Correct me if I am wrong @pylipp .

[pylipp]

Thanks for the investigation! So you're developing on Windows? Maybe it's a peculiarity of requests to use simplejson on Windows. I'm solely developing on Linux but it was already noticed that the tests don't pass on Windows (https://github.com/pylipp/financeager/issues/11#issuecomment-542212160, and #36). So I'd rather not include the ValueError catch here, but in the context of a new branch that seeks to make all tests cross-platform. Do you have issues running other tests? Also, could you set up your development env acc. to the README?

[pylipp]

I just learnt that simplejson is included as the json module in modern (>3.0) Python versions which are the only ones that financeager supports. Is it possible that your test environment is not isolated such that simplejson somehow gets imported with requests (see https://github.com/psf/requests/blob/v2.22.0/requests/compat.py#L29)?

[Joel-hanson]

Thanks for the investigation! So you're developing on Windows? Maybe it's a peculiarity of requests to use simplejson on Windows.

I am using a mac @pylipp

So I'd rather not include the ValueError catch here, but in the context of a new branch that seeks to make all tests cross-platform.

I will revert that change and update this pull request

Do you have issues running other tests?

I have 11 failures in the test

could you set up your development env acc. to the README?

Did you mean to make a writeup to show how we can run the code in the local machine?

[Joel-hanson]

I just learnt that simplejson is included as the json module in modern (>3.0) Python versions which are the only ones that financeager supports. Is it possible that your test environment is not isolated such that simplejson somehow gets imported with requests (see https://github.com/psf/requests/blob/v2.22.0/requests/compat.py#L29)?

Thanks for clarifying this @pylipp

[pylipp]

Thanks for the investigation! So you're developing on Windows? Maybe it's a peculiarity of requests to use simplejson on Windows.

I am using a mac @pylipp

Alright! Then testing should be fine due to #37.

So I'd rather not include the ValueError catch here, but in the context of a new branch that seeks to make all tests cross-platform.

I will revert that change and update this pull request

Thanks :)

Do you have issues running other tests?

I have 11 failures in the test

Okay, that's funny!

could you set up your development env acc. to the README?

Did you mean to make a writeup to show how we can run the code in the local machine?

I was talking about whether you set up your development environment according to the 'Contributing' section of the README (including having a venv and dev tools installed)?

[Joel-hanson]

I was talking about whether you set up your development environment according to the 'Contributing' section of the README (including having a venv and dev tools installed)?

Understood will do this @pylipp

[pylipp]

Great, thanks for being so patient :D