Skip to content

Fix test_callable_spec to handle keyword only parameters#2040

Open
the-allanc wants to merge 2 commits intocherrypy:mainfrom
the-allanc:use-inspect-signature-for-testing-callable-spec
Open

Fix test_callable_spec to handle keyword only parameters#2040
the-allanc wants to merge 2 commits intocherrypy:mainfrom
the-allanc:use-inspect-signature-for-testing-callable-spec

Conversation

@the-allanc
Copy link
Copy Markdown
Contributor

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 #)

None.

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

If a TypeError occurs inside the execution of the handler, and the handler has had keyword parameters passed to it, test_callable_spec will cause a 404 to be returned, rather than allowing the TypeError to be correctly propagated as a 500 response.

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

We delegate validation of the provided args and kwargs against the callable to the inspect module, which adds support for keyword-only parameters while removing the need to perform these checks directly in CherryPy. In the situation mentioned previously, this means we correctly raise a 500 error instead of returning a 404 response.

Other information:

I've kept the behaviour compatible with what was previously there regarding reporting user-friendly messages when "show_mismatched_params" is enabled (all previous tests pass), but I've also added additional checks to testParamErrors which will fail if using the current code in _cpdispatch.py. There were also some bugs in the tests which I found while making these changes that I've fixed.

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

@the-allanc the-allanc force-pushed the use-inspect-signature-for-testing-callable-spec branch from 8e49095 to c4ca792 Compare November 22, 2024 01:48
@the-allanc
Copy link
Copy Markdown
Contributor Author

the-allanc commented Nov 22, 2024

Currently tests fail in versions of Python that have reached EOL - see #2041.

@webknjaz webknjaz self-requested a review November 24, 2024 02:10
@webknjaz webknjaz self-assigned this Jan 21, 2025
@webknjaz
Copy link
Copy Markdown
Member

Clicking “rebase” in this PR...

Also fixed some mistakes in the tests themselves.
This is done by delegating the validation to inspect.Signature.bind,
and processing the error that it raises.
@webknjaz webknjaz force-pushed the use-inspect-signature-for-testing-callable-spec branch from c4ca792 to 8002e15 Compare April 25, 2025 11:33
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 25, 2025

Codecov Report

Attention: Patch coverage is 97.72727% with 1 line in your changes missing coverage. Please review.

Project coverage is 76.74%. Comparing base (39da5a8) to head (8002e15).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2040      +/-   ##
==========================================
- Coverage   76.74%   76.74%   -0.01%     
==========================================
  Files         107      107              
  Lines       13986    13956      -30     
  Branches     1702     1689      -13     
==========================================
- Hits        10734    10710      -24     
+ Misses       2641     2635       -6     
  Partials      611      611              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

cherrypy/test/test_request_obj.py::RequestObjectTests::testParamErrors is broken on Python 3.12. It probably needs some compat fallbacks for >= 3.12. I haven't yet added 3.13 to the CI but I expect it to fail the same.

@webknjaz webknjaz changed the title Fix test_callable_spec to handle keyword only parameters. Fix test_callable_spec to handle keyword only parameters Apr 25, 2025
@webknjaz
Copy link
Copy Markdown
Member

@the-allanc FYI it looks like the email in the commits (allan AT sixtyten PERIOD org) isn't verified in your GitHub account. So GH isn't attributing the commits to you. You can fix that @ https://github.com/settings/emails or by changing the committer with git commit --author='...'.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants