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

tests for request event and interception with redirects #3994

Merged

Conversation

tjenkinson
Copy link
Contributor

@tjenkinson tjenkinson commented Sep 28, 2020

See #3993

This adds a currently failing test for intercepting a request that redirects. The interceptor is not called for the redirect.

@pavelfeldman
Copy link
Member

The network interception in Playwright is implemented on the Browser -> Network stack boundary. Once the request is in the network stack, it is going to handle the redirects and report them, but not allow intercepting them. So while this is a limitation, it is currently considered to be 'as-designed'.

@tjenkinson
Copy link
Contributor Author

Ah right that’s unfortunate. The test for redirects and the request event might still be useful. Happy to remove the intercept one. Also would be good to clarify this in the docs.

Is there a way of providing an allow list on urls that can be accessed (including redirects) and block the rest?

Thanks

test/network-request.spec.ts Outdated Show resolved Hide resolved
test/network-request.spec.ts Outdated Show resolved Hide resolved
test/network-request.spec.ts Outdated Show resolved Hide resolved
@tjenkinson tjenkinson force-pushed the failing-test-for-intercept-redirect branch from 4ca7d17 to 7c00b0a Compare September 30, 2020 19:46
@tjenkinson tjenkinson changed the title Failing test for intercept redirect tests for request event and interception with redirects Sep 30, 2020
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Looks good, just a minor comment in the docs.

docs/api.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dgozman dgozman left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! Looks great, merging it 🚀

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.

3 participants