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

🧪 Integrate Hypothesis in tests #860

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

webknjaz
Copy link
Member

What do these changes do?

$sbj

Are there changes in behavior for the user?

Nah

Related issue number

Nope

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1456dd6) 99.33% compared to head (98412df) 99.48%.
Report is 1 commits behind head on master.

❗ Current head 98412df differs from pull request most recent head 3247e27. Consider uploading reports for the commit 3247e27 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
+ Coverage   99.33%   99.48%   +0.14%     
==========================================
  Files          17        4      -13     
  Lines        3315      772    -2543     
  Branches      323        0     -323     
==========================================
- Hits         3293      768    -2525     
+ Misses         22        4      -18     
Flag Coverage Δ
unit 99.48% <ø> (+0.23%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@webknjaz webknjaz force-pushed the testing/hypothesis branch 5 times, most recently from e6af87c to 23f4ed0 Compare April 26, 2023 20:37
@webknjaz
Copy link
Member Author

webknjaz commented Jun 6, 2023

@mjpieters I started looking into using Hypothesis during PyCon (the maintainer helped me start).
Does this look like a yarl bug to you? https://github.com/aio-libs/yarl/actions/runs/4813151119/jobs/8569304512?pr=860#step:7:1489

@mjpieters
Copy link
Contributor

@mjpieters I started looking into using Hypothesis during PyCon (the maintainer helped me start). Does this look like a yarl bug to you? https://github.com/aio-libs/yarl/actions/runs/4813151119/jobs/8569304512?pr=860#step:7:1489

I was going to look into that since I found this PR! It does look like %30 is not being decoded properly, so that does look an awful lot like a bug.

@mjpieters
Copy link
Contributor

mjpieters commented Jun 7, 2023

Actually, no this is not a bug in Yarl; it is a bug in your Hypothesis tests. unsafe contains '0':

test_quote_unquote_parameter(
    quoter=yarl._quoting_c._Quoter,
    unquoter=yarl._quoting_c._Unquoter,
    text_input='0',
    safe='',
    unsafe='0',   # <--- 0 is unsafe
    protected='',
    qs=False,
    requote=False,
)

With 0 unsafe, the unquoter must return %30.

Either not tell Hypothesis to provide input to unsafe, or account for characters in unsafe when comparing input and output.

@webknjaz
Copy link
Member Author

webknjaz commented Jun 7, 2023

@mjpieters yeah, I never used these and wasn't sure about the semantics which is why I asked you. My objective is to make the foundation for adding more Hypothesis testing in the future and so the contributors could look at the examples...

@webknjaz webknjaz force-pushed the testing/hypothesis branch 4 times, most recently from 6d4483d to b68147b Compare June 8, 2023 02:08
tests/test_quoting.py Dismissed Show dismissed Hide dismissed
@webknjaz
Copy link
Member Author

webknjaz commented Jun 8, 2023

@webknjaz webknjaz force-pushed the testing/hypothesis branch 2 times, most recently from 958a7d4 to 19b85a6 Compare June 15, 2023 16:13
@webknjaz webknjaz force-pushed the testing/hypothesis branch 6 times, most recently from 98412df to 37a9f48 Compare November 19, 2023 21:26
@webknjaz webknjaz force-pushed the testing/hypothesis branch 4 times, most recently from d0a3aff to c3036c3 Compare November 20, 2023 22:26
@webknjaz
Copy link
Member Author

@mjpieters do you have ideas on how to best get this integrated finally? What extra constraints do we need in these tests?

@webknjaz webknjaz force-pushed the testing/hypothesis branch 3 times, most recently from 914bd16 to f0564f9 Compare November 25, 2023 02:50
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.

None yet

2 participants