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

Keywords for TRACE and CONNECT HTTP methods #390

Merged
merged 15 commits into from
May 28, 2024

Conversation

PaulBrandUWV
Copy link
Contributor

Adds keywords for CONNECT and TRACE HTTP methods.
Fixes documentation for default value for allow_redirects.
Limits versions for flask and werkzeug to make sure local http server works.
Fixes some unit tests related to optional allow_redirect argument.
Updates documentation.

All unit tests pass
All acceptance tests pass

@lucagiove
Copy link
Member

On my machine there are failures on all test_RequestsOnSessionKeywords
Seems mock is not working and the test tries to really connect to the network.

utests/test_RequestsOnSessionKeywords.py:16 (test_common_request_file_descriptor_closing)
self = <urllib3.connection.HTTPConnection object at 0x108753640>

    def _new_conn(self):
        """Establish a socket connection and set nodelay settings on it.
    
        :return: New socket connection.
        """
        extra_kw = {}
        if self.source_address:
            extra_kw["source_address"] = self.source_address
    
        if self.socket_options:
            extra_kw["socket_options"] = self.socket_options
    
        try:
>           conn = connection.create_connection(
                (self._dns_host, self.port), self.timeout, **extra_kw
            )

/Users/luca/.virtualenvs/robotframework-requests/lib/python3.9/site-packages/urllib3/connection.py:174: 

@lucagiove lucagiove self-requested a review May 3, 2024 23:19
@lucagiove lucagiove self-assigned this May 3, 2024
@lucagiove lucagiove added this to the 1.0 milestone May 3, 2024
@lucagiove
Copy link
Member

By the way it's a nice work. Thanks, I should try to understand why GitHub pipelines stop running... :(

@PaulBrandUWV
Copy link
Contributor Author

I think approval from a maintainer will make the workflows run?
It seems you are using a pretty old version of python (3.9). I ran all tests on 3.12 with a fresh virtualenv.

@lucagiove
Copy link
Member

I think approval from a maintainer will make the workflows run? It seems you are using a pretty old version of python (3.9). I ran all tests on 3.12 with a fresh virtualenv.

I approved but I had issues also with my commits.. I'll check it out if you have some suggestion is welcomed.
Anyhow we should cover multiple python versions maybe we can review a bit the supported matrix.

@PaulBrandUWV
Copy link
Contributor Author

@lucagiove Sorry, it was my mistake. Not sure which unit test I ran on my system before, they should not have passed.
I fixed them in my latest commit.

I think support for python 2 can be dropped. Robot Framework currently supports >=3.8.

@lucagiove
Copy link
Member

Yes I agree I should drop some legacy python versions.
Actually I should have a matrix between robot and python versions on different OS.

@PaulBrandUWV
Copy link
Contributor Author

PaulBrandUWV commented May 6, 2024

Yes I agree I should drop some legacy python versions. Actually I should have a matrix between robot and python versions on different OS.

I've made a pull request that removes support for python 2. Yeah compatibility with different Robot Framework versions would be nice. What about different versions of requests?

Copy link

codecov bot commented May 7, 2024

Codecov Report

Attention: Patch coverage is 90.32258% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 88.60%. Comparing base (a9e8f91) to head (d93c3a4).

Files Patch % Lines
src/RequestsLibrary/utils.py 33.33% 6 Missing ⚠️
src/RequestsLibrary/SessionKeywords.py 83.33% 3 Missing and 1 partial ⚠️
src/RequestsLibrary/compat.py 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
+ Coverage   83.98%   88.60%   +4.62%     
==========================================
  Files           9        9              
  Lines         437      465      +28     
  Branches      102      116      +14     
==========================================
+ Hits          367      412      +45     
+ Misses         65       51      -14     
+ Partials        5        2       -3     
Flag Coverage Δ
acceptance 84.94% <89.51%> (+0.96%) ⬆️
unit 59.56% <57.25%> (?)

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.

@PaulBrandUWV
Copy link
Contributor Author

@lucagiove I just merged with master after the last two PR's were merged, Thanks! This PR should be good to be merged now.

@lucagiove
Copy link
Member

Yeah compatibility with different Robot Framework versions would be nice. What about different versions of requests?

Yes you're right.

@lucagiove
Copy link
Member

@lucagiove I just merged with master after the last two PR's were merged, Thanks! This PR should be good to be merged now.

Great, now let me have a deeper look at the code.

Can I ask why you added support for these methods? You needed to run tests with them?

@PaulBrandUWV
Copy link
Contributor Author

@lucagiove I just merged with master after the last two PR's were merged, Thanks! This PR should be good to be merged now.

Great, now let me have a deeper look at the code.

Can I ask why you added support for these methods? You needed to run tests with them?

Yeah I needed some tests to make sure our rest api did not allow these methods (http 405)

@PaulBrandUWV
Copy link
Contributor Author

@lucagiove did you have a chance to check the code? I'm testing with a modified version of requestslibrary now but would like to stick to the stable or pre release version

src/RequestsLibrary/RequestsKeywords.py Outdated Show resolved Hide resolved
src/RequestsLibrary/RequestsKeywords.py Outdated Show resolved Hide resolved
src/RequestsLibrary/RequestsKeywords.py Outdated Show resolved Hide resolved
src/RequestsLibrary/RequestsOnSessionKeywords.py Outdated Show resolved Hide resolved
src/RequestsLibrary/RequestsOnSessionKeywords.py Outdated Show resolved Hide resolved
@PaulBrandUWV
Copy link
Contributor Author

I fixed the default values for "allow_redirection" argument. Only HEAD and HEAD on session keywords do not allow redirects by default just like python requests.

Copy link
Member

@lucagiove lucagiove left a comment

Choose a reason for hiding this comment

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

ouch now it is a quite big review... 😅

@PaulBrandUWV
Copy link
Contributor Author

ouch now it is a quite big review... 😅

Yeah, most of the changed lines are formatting. Maybe i should have done the changes to the acceptance tests in a separate PR.

atests/__init__.robot Outdated Show resolved Hide resolved
atests/res_setup.robot Show resolved Hide resolved
src/RequestsLibrary/RequestsKeywords.py Show resolved Hide resolved
@lucagiove lucagiove merged commit 365cbcf into MarketSquare:master May 28, 2024
8 checks passed
@PaulBrandUWV
Copy link
Contributor Author

@lucagiove Thanks!

@PaulBrandUWV PaulBrandUWV deleted the new_http_methods branch June 2, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants