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

feat: unblock event queue when network events are blocked #1409

Merged
merged 18 commits into from
Oct 10, 2023

Conversation

thiagowfx
Copy link
Contributor

@thiagowfx thiagowfx commented Oct 9, 2023

Furthermore, update some E2E tests so that they do not rely on CDP events.

Bug #644, #1080
Co-authored-by: Maksim Sadym [email protected]

Re-created from #1400.

@thiagowfx thiagowfx force-pushed the thiagowfx/network-events branch 2 times, most recently from 601591e to 8faae96 Compare October 9, 2023 13:59
@thiagowfx thiagowfx force-pushed the thiagowfx/network-events branch 7 times, most recently from bd95212 to 9aa5b47 Compare October 10, 2023 11:51
@thiagowfx thiagowfx marked this pull request as ready for review October 10, 2023 11:53
@thiagowfx
Copy link
Contributor Author

It's just missing "continue request", but sending out for review already.

@@ -143,7 +130,7 @@ export class CdpTarget {
// XXX: #1080: Do not always enable the network domain globally.
// TODO: enable Network domain for OOPiF targets.
this.#cdpClient.sendCommand('Network.enable'),
this.fetchApply(),
this.fetchEnable(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate? This change is a no-op.

The more general issue you're referring to is: #1080

[
{
"type": "string",
"pattern": "https://www.example.com/",
Copy link
Collaborator

@sadym-chromium sadym-chromium Oct 10, 2023

Choose a reason for hiding this comment

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

Can you use a fixture example_url instead of const example.com?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not possible to use fixtures in parameters: pytest-dev/pytest#349

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could expose a global string instead of a fixture. Or we could break the test down into multiple functions so that it is not parameterized anymore. Otherwise it's not really possible at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I forgot to mention a third possibility: We could use a pytest plug-in that allows so. Do you have a preference? See the upstream issue above.

@thiagowfx thiagowfx merged commit e94f79d into main Oct 10, 2023
15 of 16 checks passed
@thiagowfx thiagowfx deleted the thiagowfx/network-events branch October 10, 2023 16:46
jrandolf-2 pushed a commit that referenced this pull request Oct 16, 2023
🤖 I have created a release *beep* *boop*
---


##
[0.4.32](chromium-bidi-v0.4.31...chromium-bidi-v0.4.32)
(2023-10-16)


### Features

* add quality for `webp`
([#1426](#1426))
([d514bf9](d514bf9))
* implement device pixel ratio changes
([#1422](#1422))
([49f6dee](49f6dee))
* implement document origin screenshots
([#1427](#1427))
([b952297](b952297))
* **network interception:** implement continue request
([#1331](#1331))
([8a935b9](8a935b9)),
closes
[#644](#644)
* session handling refactoring. Step 1
([#1385](#1385))
([8fe37b9](8fe37b9))
* unblock event queue when network events are blocked
([#1409](#1409))
([e94f79d](e94f79d))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
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