Tests for DevTools protocol for DirectSocket events [chromium/src : main]
Vlad Krot (Gerrit)
Message from Vlad Krot![]( "Open in Gerrit")
Set Ready For Review
Related details
- Vlad Krot
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
"prefix": "inspector-protocol-direct-sockets",Should I add this test suite to TestLists filters?
third_party/blink/web_tests/TestLists/rel-ready.blink_wpt_tests.filter
third_party/blink/web_tests/TestLists/content_shell.filter
Related details
- Andrey Kosyakov
- Dominic Farolino
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
"prefix": "inspector-protocol-direct-sockets",Should I add this test suite to TestLists filters?
third_party/blink/web_tests/TestLists/rel-ready.blink_wpt_tests.filter
third_party/blink/web_tests/TestLists/content_shell.filter
Acknowledged
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 12 comments![]( "Open in Gerrit")
"prefix": "inspector-protocol-direct-sockets",Should I add this test suite to TestLists filters?
third_party/blink/web_tests/TestLists/rel-ready.blink_wpt_tests.filter
third_party/blink/web_tests/TestLists/content_shell.filter
No, these two are just for WPTs, which these tests are not.
"args": ["--isolated-context-origins=https://devtools.oopif.test"],Can we just reuse the origin and the suite above? I.e. just add one more `bases` entry there and use https://web-platform.test?
header("Origin-Agent-Cluster: ?0");These aren't really required, are they? I mean, I tried removing them and the tests still pass, I think the command line switch is the only requirement.
async function openTCP() {nit: instead of putting code into the server-side resources, we could have a single empty HTML with proper headers and evaluate the rest on the test side -- this way you would save on boilerplate and have all code required by test in a single place. And given the comment above, we could probably just navigate to `https://web-platform.test/inspector-protocol/resources/inspector-protocol-page.html` (i.e. a blank page from the right origin).
var {page, session, dp} = await testRunner.startURL(nit: s/var/const/
testRunner.log('Test started');Let's ditch extra progress messages (this one and the one below).
session.evaluateAsync('openTCP()');... so as per comment in the PHP file, this would be just
```
session.evaluate(`
new TCPSocket("127.0.0.1", 468, {
noDelay: true, receiveBufferSize: 10011,
sendBufferSize: 10022, keepAliveDelay: 10033, dnsQueryType: "ipv6"
});
`);
```
(note no need for awaiting `clientSocket.opened` since you're not awaiting `evaluateAsync()` call either. And this is also why it can be just a `session.evaluate()`).
})();just `const createdEventPromise = dp.Network.onceDirectTCPSocketCreated();`, please :-)
var {page, session, dp} = await testRunner.startURL(s/var/const/, please!
let createdEventPromise = (async () => {ditto, just
`const createdEventPromise = dp.Network.onceDirectTCPSocketCreated()` etc.
var {page, session, dp} = await testRunner.startURL(ditto
let createdEventPromise = (async () => {ditto.
Related details
- Dominic Farolino
- Vlad Krot
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 11 comments![]( "Open in Gerrit")
"args": ["--isolated-context-origins=https://devtools.oopif.test"],Can we just reuse the origin and the suite above? I.e. just add one more `bases` entry there and use https://web-platform.test?
It doesn't work with custom php page which is necessary at least for headers and for initiator (not strictly neccessary)
header("Origin-Agent-Cluster: ?0");These aren't really required, are they? I mean, I tried removing them and the tests still pass, I think the command line switch is the only requirement.
Are you sure? I tried without those headers and it crashes. Maybe you have DCHECKS disabled?
More over it might create troubles in the future, because IWA always expects these headers to be set.
https://docs.google.com/document/d/1HfPh1UNdjhbPshkrQlIWPzOJ4DJsuJKzIS92-qoxKnI/edit?resourcekey=0-yad_ljSUblW7wQt0pZAphQ&tab=t.0
`STDERR: [40822:1:0418/090635.373533:FATAL:third_party/blink/renderer/core/execution_context/agent.cc:108] DCHECK failed: is_isolated_context == value (0 vs. 1)`
async function openTCP() {nit: instead of putting code into the server-side resources, we could have a single empty HTML with proper headers and evaluate the rest on the test side -- this way you would save on boilerplate and have all code required by test in a single place. And given the comment above, we could probably just navigate to `https://web-platform.test/inspector-protocol/resources/inspector-protocol-page.html` (i.e. a blank page from the right origin).
Good idea, thanks!
But for the success case we must use js in php, because it checks initiator. And if the code is loaded via evalJs then functionName and url for initiator are null.
var {page, session, dp} = await testRunner.startURL(Vlad Krotnit: s/var/const/
Done
Let's ditch extra progress messages (this one and the one below).
Done
... so as per comment in the PHP file, this would be just
```
session.evaluate(`
new TCPSocket("127.0.0.1", 468, {
noDelay: true, receiveBufferSize: 10011,
sendBufferSize: 10022, keepAliveDelay: 10033, dnsQueryType: "ipv6"
});
`);
```
(note no need for awaiting `clientSocket.opened` since you're not awaiting `evaluateAsync()` call either. And this is also why it can be just a `session.evaluate()`).
Done
just `const createdEventPromise = dp.Network.onceDirectTCPSocketCreated();`, please :-)
I thought that it prevents race condition if an event fired after there's a subscription to it.
But I tested, it works the way you suggested.
Btw, why there's no race condition?
var {page, session, dp} = await testRunner.startURL(Vlad Krots/var/const/, please!
Done
ditto, just
`const createdEventPromise = dp.Network.onceDirectTCPSocketCreated()` etc.
Done
var {page, session, dp} = await testRunner.startURL(Vlad Krotditto
Done
let createdEventPromise = (async () => {Vlad Krotditto.
Done
Related details
- Andrey Kosyakov
- Dominic Farolino
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 4 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
4 comments
"args": ["--isolated-context-origins=https://devtools.oopif.test"],Vlad KrotCan we just reuse the origin and the suite above? I.e. just add one more `bases` entry there and use https://web-platform.test?
It doesn't work with custom php page which is necessary at least for headers and for initiator (not strictly neccessary)
Acknowledged
header("Origin-Agent-Cluster: ?0");Vlad KrotThese aren't really required, are they? I mean, I tried removing them and the tests still pass, I think the command line switch is the only requirement.
Are you sure? I tried without those headers and it crashes. Maybe you have DCHECKS disabled?
More over it might create troubles in the future, because IWA always expects these headers to be set.
https://docs.google.com/document/d/1HfPh1UNdjhbPshkrQlIWPzOJ4DJsuJKzIS92-qoxKnI/edit?resourcekey=0-yad_ljSUblW7wQt0pZAphQ&tab=t.0
`STDERR: [40822:1:0418/090635.373533:FATAL:third_party/blink/renderer/core/execution_context/agent.cc:108] DCHECK failed: is_isolated_context == value (0 vs. 1)`
Ah, indeed! Well, that's a bit [unfortunate](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#check_dcheck_and-notreached) to have a check like this as a DCHECK() -- this is not exactly an invariant.
async function openTCP() {Vlad Krotnit: instead of putting code into the server-side resources, we could have a single empty HTML with proper headers and evaluate the rest on the test side -- this way you would save on boilerplate and have all code required by test in a single place. And given the comment above, we could probably just navigate to `https://web-platform.test/inspector-protocol/resources/inspector-protocol-page.html` (i.e. a blank page from the right origin).
Good idea, thanks!
But for the success case we must use js in php, because it checks initiator. And if the code is loaded via evalJs then functionName and url for initiator are null.
```
session.evaluate(`
function openTCP() {
...
}
openTCP();
//# sourceURL=tcp-socket-success.php
`);
```
would take care of this, but feel free to keep as is. Here's an [example](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/inspector-protocol/worker/worker-constructor-async-stacks.js;l=20).
Related details
- Dominic Farolino
- Vlad Krot
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
"expires": "never"Please see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VIRTUAL_OWNERS;l=31-32?q=VIRTUAL_OWNERS and add an explanation for this in the CL description.
Related details
- Vlad Krot
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 3 comments![]( "Open in Gerrit")
"expires": "never"Please see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VIRTUAL_OWNERS;l=31-32?q=VIRTUAL_OWNERS and add an explanation for this in the CL description.
I have added to explanation to the CL
"The tests are never meant to expire, because they test stable DirectSocket API and Chrome Dev Tools Protocol integration. There are no plans to remove the features thus the tests must stay."
header("Origin-Agent-Cluster: ?0");Vlad KrotThese aren't really required, are they? I mean, I tried removing them and the tests still pass, I think the command line switch is the only requirement.
Andrey KosyakovAre you sure? I tried without those headers and it crashes. Maybe you have DCHECKS disabled?
More over it might create troubles in the future, because IWA always expects these headers to be set.
https://docs.google.com/document/d/1HfPh1UNdjhbPshkrQlIWPzOJ4DJsuJKzIS92-qoxKnI/edit?resourcekey=0-yad_ljSUblW7wQt0pZAphQ&tab=t.0
`STDERR: [40822:1:0418/090635.373533:FATAL:third_party/blink/renderer/core/execution_context/agent.cc:108] DCHECK failed: is_isolated_context == value (0 vs. 1)`
Ah, indeed! Well, that's a bit [unfortunate](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#check_dcheck_and-notreached) to have a check like this as a DCHECK() -- this is not exactly an invariant.
agree. better to make as CHECK().
async function openTCP() {Vlad Krotnit: instead of putting code into the server-side resources, we could have a single empty HTML with proper headers and evaluate the rest on the test side -- this way you would save on boilerplate and have all code required by test in a single place. And given the comment above, we could probably just navigate to `https://web-platform.test/inspector-protocol/resources/inspector-protocol-page.html` (i.e. a blank page from the right origin).
Andrey KosyakovGood idea, thanks!
But for the success case we must use js in php, because it checks initiator. And if the code is loaded via evalJs then functionName and url for initiator are null.
```
session.evaluate(`
function openTCP() {
...
}
openTCP();
//# sourceURL=tcp-socket-success.php
`);
```would take care of this, but feel free to keep as is. Here's an [example](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/inspector-protocol/worker/worker-constructor-async-stacks.js;l=20).
Tried to do it like this, doesn't work, initiator fields are still null.
Related details
- Dominic Farolino
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino added 1 comment![]( "Open in Gerrit")
"expires": "never"Vlad KrotPlease see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VIRTUAL_OWNERS;l=31-32?q=VIRTUAL_OWNERS and add an explanation for this in the CL description.
I have added to explanation to the CL
"The tests are never meant to expire, because they test stable DirectSocket API and Chrome Dev Tools Protocol integration. There are no plans to remove the features thus the tests must stay."
I guess I'm confused why we need VTS for them then? We have many stable features that are just always around... and we just write tests for them that run on the CI. Why do these tests need a special virtual configuration? Maybe I misunderstand the feature.
Related details
- Vlad Krot
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot added 1 comment![]( "Open in Gerrit")
"expires": "never"Vlad KrotPlease see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VIRTUAL_OWNERS;l=31-32?q=VIRTUAL_OWNERS and add an explanation for this in the CL description.
Dominic FarolinoI have added to explanation to the CL
"The tests are never meant to expire, because they test stable DirectSocket API and Chrome Dev Tools Protocol integration. There are no plans to remove the features thus the tests must stay."
I guess I'm confused why we need VTS for them then? We have many stable features that are just always around... and we just write tests for them that run on the CI. Why do these tests need a special virtual configuration? Maybe I misunderstand the feature.
Because it is impossible to run the test without Chrome content shell command line flag - "--isolated-context-origins=https://devtools.oopif.test" since DirectSocket API is available only in Isolated Context.
Related details
- Dominic Farolino
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Farolino (Gerrit)
Dominic Farolino voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominic Farolino
| Code-Review | +1 |
2 comments
"expires": "never"Vlad KrotPlease see https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/VIRTUAL_OWNERS;l=31-32?q=VIRTUAL_OWNERS and add an explanation for this in the CL description.
Dominic FarolinoI have added to explanation to the CL
"The tests are never meant to expire, because they test stable DirectSocket API and Chrome Dev Tools Protocol integration. There are no plans to remove the features thus the tests must stay."
Vlad KrotI guess I'm confused why we need VTS for them then? We have many stable features that are just always around... and we just write tests for them that run on the CI. Why do these tests need a special virtual configuration? Maybe I misunderstand the feature.
Because it is impossible to run the test without Chrome content shell command line flag - "--isolated-context-origins=https://devtools.oopif.test" since DirectSocket API is available only in Isolated Context.
- Vlad Krot
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vlad Krot (Gerrit)
Vlad Krot voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Tests for DevTools protocol for DirectSocket events
These tests are for changes introduced in
https://chromium-review.googlesource.com/c/chromium/src/+/6434437
https://chromium-review.googlesource.com/c/chromium/src/+/6308994
The tests are never meant to expire, because they test stable
DirectSocket API and Chrome Dev Tools Protocol integration. There are no
plans to remove the features thus the tests must stay.
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/resources/tcp-socket-default.php
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/resources/tcp-socket-success.php
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-abort-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-abort.js
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-chunk-error-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-chunk-error.js
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-success-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/network/direct-sockets/tcp-socket-success.js
- A third_party/blink/web_tests/virtual/inspector-protocol-direct-sockets/http/tests/inspector-protocol/network/direct-sockets/README.txt
Code-Review: +1 by Dominic Farolino, +1 by Andrey Kosyakov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |