Skip to content

Regression tests for issue 312 #316

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Aug 24, 2020
Merged

Regression tests for issue 312 #316

merged 10 commits into from
Aug 24, 2020

Conversation

cyraxjoe
Copy link
Contributor

@cyraxjoe cyraxjoe commented Aug 23, 2020 •
edited by webknjaz
Loading

❓ What kind of change does this PR introduce?

  • 🐞 bug fix
  • 🐣 feature
  • 📋 docs update
  • 📋 tests/coverage improvement
  • 📋 refactoring
  • 💥 other

📋 What is the related issue number (starting with #)

#312

❓ What is the current behavior? (You can also link to an open issue here)

Errors that causes issues #312 weren't detected by the testsuite.

❓ What is the new behavior (if this is a feature change)?

Detect the errors that caused issues #312

📋 Other information:

This PR implements the tests that are meant to be passed by PR #313, it expands the test coverage of the coonections module.

📋 Checklist:

  • I think the code is well written
  • I wrote good commit messages
  • I have squashed related commits together after the changes have been approved
  • Unit tests for the changes exist
  • Integration tests for the changes exist (if applicable)
  • I used the same coding conventions as the rest of the project
  • The new code doesn't generate linter offenses
  • Documentation reflects the changes
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences

This change is Reviewable

@cyraxjoe
Monkeypatch the server error_log method to verify the content during …
853706e 
…tests.

Using the new error_log mock class inspect the value during the keep
alive tests, previously ignoring the log caused the regression of
issue cherrypy#313.
@cyraxjoe
Generalize the error_log verification for the testing_server fixture.
7134c90
@cyraxjoe
Add test to verify the error handling segment of ConnectionManager.ge…
3daf3e0 
…t_conn
@cyraxjoe cyraxjoe mentioned this pull request Aug 23, 2020
Merged
15 tasks
webknjaz
webknjaz reviewed Aug 23, 2020
View reviewed changes
webknjaz
webknjaz reviewed Aug 23, 2020
View reviewed changes
webknjaz
webknjaz reviewed Aug 23, 2020
View reviewed changes
webknjaz
webknjaz reviewed Aug 23, 2020
View reviewed changes
webknjaz
webknjaz reviewed Aug 23, 2020
View reviewed changes
webknjaz
webknjaz reviewed Aug 23, 2020
View reviewed changes
cyraxjoe and others added 4 commits August 23, 2020 15:59
@cyraxjoe @webknjaz
Apply some suggestions from code review
f8a24a3 
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
@cyraxjoe
Implement PR review feedback.
9650593 
Mainly moving class definition to the top level and adding more docstring.

The most notable thing is the replacement of the fixture `testing_server`
to `raw_testing_server` and move the error_log patching in a new one
that will include that functionality using the old name `testing_server`.
@cyraxjoe
Use the same name for the namedtuple used in the ErrorLogMonitor class.
4f044fe
@cyraxjoe
Small refactor to conform to review comments.
a4cfe02
@cyraxjoe
Copy link
Contributor Author

@webknjaz I think I implemented all of the comments. Just a note on using assert instead of rising the exception.. it seems that the errors messages in the test run are less readable, the traceback is not fully shown. Not sure if that is configurable by the pytest invocation.

Compare this run https://github.com/cherrypy/cheroot/runs/1018974455 vs https://github.com/cherrypy/cheroot/runs/1019226287

webknjaz
webknjaz reviewed Aug 24, 2020
View reviewed changes
cheroot/test/test_conn.py
conn.close()
self.socket_closed = True

return self.original_get_map()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about self.original_get_map() being called twice. Can it return different values for each invocation? Should it be called once and the result reused?

@webknjaz
Copy link
Member

the traceback is not fully shown. Not sure if that is configurable by the pytest invocation.

I think you can make it more verbose with something like -vvvvv --full-trace

@webknjaz webknjaz merged commit 7644616 into cherrypy:master Aug 24, 2020
@webknjaz webknjaz linked an issue Aug 24, 2020 that may be closed by this pull request
timed_out_connections - dictionary changed size during iteration #312
Closed
3 tasks
@webknjaz
Copy link
Member

I noticed some Python 2 issues after the merge, trying to address them in master.

webknjaz added a commit that referenced this pull request Aug 24, 2020
@webknjaz
Refactor error log exit check in test_conn
ac5ad86 
This change allows pytest to show the local variables' values in
the error summary.

Ref: #316 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@webknjaz webknjaz webknjaz left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

timed_out_connections - dictionary changed size during iteration
2 participants
@cyraxjoe @webknjaz