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

Add config tests #399

Merged
merged 8 commits into from
Sep 4, 2019
Merged

Add config tests #399

merged 8 commits into from
Sep 4, 2019

Conversation

cknv
Copy link
Contributor

@cknv cknv commented Aug 7, 2019

As mentioned in #102 there was a need for some more test coverage of the config class, which seems like a fairly lowing hanging fruit, possibly low enough that I could manage to pick it.

I used the latest codecov report of the class to see what lines was missing coverage and tried to target those as best I could. I went with the naive way of just asserting what the code returned, so it is not impossible (perhaps likely, even) that I am mistaken about the behaviour of the class.

Locally I have some failing tests in the test_statreload.py file, but as I have yet to make any changes to the code itself, I suspect there where there when I cloned the repo. I could have a look at what those are, but I have barely looked past the config class.

tests/test_config.py Outdated Show resolved Hide resolved
@cknv
Copy link
Contributor Author

cknv commented Aug 27, 2019

I have rebased onto the latest master which should make the test suite pass.

@cknv
Copy link
Contributor Author

cknv commented Aug 27, 2019

I see the previously failing tests was masking some actual problems, and have a bit more work cut out for me with regards to dealing with python 3.5 and the permissions of the certificates on windows.

@cknv
Copy link
Contributor Author

cknv commented Aug 27, 2019

The cert + windows problems seems to stem from the fact that NamedTemporaryFile is not able to be re-opened on Windows NT and later (or so is my understanding of the docs at least) it shouldn't be too hard to fix that too.

cknv added 3 commits August 27, 2019 20:34
Using NamedTemporaryFile seems to cause problems on Windows builds
as they are not able to be reopened later on, instead it is
possible to use the pytest fixture tmp_path to build more looking
tmp files, and just return the filename, which works fine as only
the names was being used anyway.
@cknv
Copy link
Contributor Author

cknv commented Aug 27, 2019

The windows build is still failing, this time when getting "/" of test_request_logging.

Judging from the logs on travis, there is an extra task spawned onto the loop for some reason and the mock loop only runs a single coroutine for those tests. It started happening after the change from NamedTemporaryFile to using tmp_path to just build paths with.

I don't have a windows machine at hand to replicate this with, so if someone can see the connection between the changes I did and the test, I would be very happy to continue, but currently I am just hitting the wall here.

@jlaine
Copy link
Contributor

jlaine commented Aug 28, 2019

CI failures in test_request_logging may be a symptom of what I reported in #406 : this test is subject to "pollution" from exceptions raised by other tests. I'm starting to think that clearing the log capture at the beginning of this test would be a good idea.

@cknv
Copy link
Contributor Author

cknv commented Sep 3, 2019

Oh yeah @jlaine that looks very similar, and indeed I can actually see a similar log line being spewed out when running the tests locally, although that does not make it fail for some reason (running Arch locally here with Python 3.7.4 and uvloop 0.13.0).
Could it be that maybe its actually some stray asyncio tasks that have been left behind by a test only to be destroyed in another ? It feels a bit like wild speculation, but I cannot shake the thought.

@jlaine
Copy link
Contributor

jlaine commented Sep 4, 2019

I have submitted a tentative patch in #418. AFAICT the issue is that if the WSGI task raises an exception, the "sender" task (in charge of sending out response messages) is left dangling.

@tomchristie
Copy link
Member

Okay, probably worth merging master into this PR, and seeing where that gets us, right?

* master:
  Relax uvloop version requirement to 0.*
  If the WSGI task blows up, allow the sender task to shut down
@cknv
Copy link
Contributor Author

cknv commented Sep 4, 2019

That seemed to do the trick. Very nice @jlaine!

Now that it is actually passing tests, we can

@tomchristie tomchristie merged commit 41c52e7 into encode:master Sep 4, 2019
@tomchristie
Copy link
Member

Lovely stuff!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants