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

Match any network filter instead of all #155

Merged
merged 3 commits into from
Nov 20, 2024

Conversation

rsmeral
Copy link
Contributor

@rsmeral rsmeral commented Nov 13, 2024

Description

enable_network is currently broken for more than one hostname.
The documentation (https://github.com/h2non/pook/blob/master/src/pook/engine.py#L91-L92) says :

If at least one hostname matches with the outgoing traffic, the request will be executed via the real network.

Currently, it tries to match all hostnames.

PR Checklist

  • I've added tests for any code changes (not sure how to test without actually calling external servers)
  • I've documented any new features (no new features)

Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

Thanks very much for the PR and for finding this bug. The change looks good. To add a test without relying on external networks, use the httpbin fixture and work from the existing test for enable_network which does not use the hostnames argument: https://github.com/h2non/pook/blob/HEAD/tests/unit/interceptors/base.py#L78-L87.

The httpbin fixture exposes an httpbin instance running within the test context, so it's safe to use it to test unmocked requests that need to actually go over the wire.

Something along the following lines could work:

@pytest.mark.pook
def test_network_filters(httpbin):
    url_404 = f"{httpbin.url}/status/404"
    url_401 = f"{httpbin.url}/status/401"
    url_403 = f"{httpbin.url}/status/403"
    pook.get(url_403).reply(200).body("hello from pook 403")
  
    network_enabled_urls = {
        url_404: 404,
        url_401: 401,
    }
    pook.enable_network(*network_enabled_urls.keys())

    # enable network filter matches , so the mock is ignored and the upstream status code is used
    for url, upstream_status in network_enabled_urls.items():
        res = urlopen(url)
        assert res.status == upstream_status

    # the 403 URL is not in the enable_network filters, so the mock matches
    res_403 = urlopen(url_403)
    assert res_403.status == 200

    # finally, assert the other two mocks are not exhausted and that two requests were "unmatched"
    assert pook.unmatched() == 2

I'd request at least one other test along the same lines that explicitly checks for a single URL as well (consider parametrizing the above example to accomplish that) and another that tests for a URL that is neither mocked nor included in the hostnames list (it should raise a pook matching failure in that case).

@rsmeral
Copy link
Contributor Author

rsmeral commented Nov 14, 2024

Thanks for the hints. I added a test hopefully covering all the bases. Let me know if you see something missing.

The tricky part is – I realized we can't just test against httpbin, because that's still just a single host. We need to test against multiple different hosts. Now it's testing against non-existent hostnames, and expecting them to fail, but to fail due to a resolution issue (test passes, because it means it tried to reach the real host), not PookNoMatches (test fails, because despite the allowed hostname, it didn't try to reach the real host).

I had issues running the whole test suite as per the README (due to various dependency errors), but I ran the individual test like this and it passed: hatch run pytest tests/unit/interceptors/httpx_test.py -k test_network_mode_multiple_hosts

@sarayourfriend
Copy link
Collaborator

I realized we can't just test against httpbin, because that's still just a single host. We need to test against multiple different hosts.

Good point, I had not considered that.

The enable_network function really just creates predicates to pass to engine.use_network_filter. So, to test this specific change of all to any, the unit test could enable_network for a hostname (httpbin), and then add a filter that checks something other than the hostname (e.g., a query parameter or header).

I'll look at pushing a change for that later today, unless you get to it before me. Apologies for the run around on this testing stuff. I've been working on getting the test coverage up, and there are still lots of big gaps! Ideally, this would already be tested, so I don't want to hold up the valuable fix on the project's broader issues.

I had issues running the whole test suite as per the README (due to various dependency errors)

Do you mind sharing what issues you ran into? If you don't mind opening a GitHub issue describing them, that would be helpful. It's a priority that the contributor docs are functional and I'm keen to fix any issues with them.

Copy link
Collaborator

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution! Had to make a small fix to the change, it turns out all([]) == True but any([]) == False, so networking mode failed in the case when no filters were given and all unmatched requests should be allowed. I modified the tests to avoid needing to rely on unresolvable domain names.

Really appreciate the contribution! I will get this deployed in a new version along with the fix for #156 sometime early next week, if not later today.

@sarayourfriend sarayourfriend merged commit b1d4a10 into h2non:master Nov 20, 2024
9 checks passed
@sarayourfriend
Copy link
Collaborator

sarayourfriend commented Nov 20, 2024

Released in 2.1.2. Thanks again @rsmeral!

@rsmeral
Copy link
Contributor Author

rsmeral commented Nov 24, 2024

Apologies for the run around on this testing stuff.

No worries at all! I wouldn't trust a library that doesn't have proper tests anyway 😄

Thanks for finishing up the tests, and for the quick turnaround on the release. And good catch on the any([])/all([])!

Do you mind sharing what issues you ran into? If you don't mind opening a GitHub issue describing them, that would be helpful. It's a priority that the contributor docs are functional and I'm keen to fix any issues with them.

I've filed the issue, but didn't have much time to debug it, so the description is quite sparse: #159

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.

2 participants