This change addresses flakiness in SSE tests caused by port conflicts
and race conditions during connection teardown.
Key changes:
- Dynamically allocates ChromeDriver ports in `sse_smoke_test.dart` and `sse_client_test.dart` instead of using a hardcoded port (4444). This avoids conflicts when tests run in parallel on CI bots.
- Adds support for locating ChromeDriver via the `CHROMEDRIVER_PATH` environment variable in both test suites.
- Updates `sse_smoke_driver.dart` to introduce a brief delay before closing connections. This ensures the client-side SseClient has sufficient time to flush its final POST request to the server, avoiding "Bad state: No element" errors.
- Modifies `test_helper.dart` to filter out `--mark_main_isolate_as_system_isolate` from arguments forwarded to spawned testee processes. This is necessary under modern `dart test` runners to ensure processes pause at start as expected.
Change-Id: Idf45cd69f366ec6b6f81cfed955aea337b277dcc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/508343
Auto-Submit: Ben Konyi <bkonyi@google.com>
Reviewed-by: Mark Zhou <markzipan@google.com>
Commit-Queue: Mark Zhou <markzipan@google.com>
In `server_connection_common.dart`, the `server removes clients that
disconnect from the API` test spawned its own Chrome instance using
`package:devtools_shared`'s `Chrome` class without isolated profiles or
essential headless flags.
This caused the test to hang or fail flakily in container environments
(like LUCI bots) and local environments: 1. Without
`--use-mock-keychain`, headless Chrome on macOS blocks on system
credential dialogs. 2. Without `--no-sandbox`, Chrome renderer processes
can crash in restricted container environments. 3. Without
`--user-data-dir`, Chrome uses the default system profile, which can
cause it to attach to an existing open Chrome instance instead of
starting a new one, meaning the process exits immediately and the test
cannot terminate it.
Fixed by directly using `package:browser_launcher`'s `Chrome` class in
the test and passing:
* `--user-data-dir` pointing to a unique temporary directory.
* `--no-first-run` and `--no-default-browser-check` to bypass welcome prompts.
* `--no-sandbox` and `--use-mock-keychain` where appropriate.
Also wrapped the test in `try-finally` to guarantee cleanup of the
temporary profile directory.
Change-Id: I6fbe5a280524b57c635ab11ef54fa07dba2794cf
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/507600
Reviewed-by: Alexander Aprelev <aam@google.com>
Auto-Submit: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
The DartRuntimeService based VM service implementation now has support
for launching DDS instances and responding to _yieldControlToDDS RPC
invocations from DDS instances.
package:vm_service test suite is ~97% passing with this change.
TEST=Local testing.
Change-Id: I2f2f1b0926845134578f08d073ed7606f1fc4173
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/490320
Reviewed-by: Jessy Yameogo <yjessy@google.com>
Reviewed-by: Nicholas Shahan <nshahan@google.com>
The `Match.operator[]` does the same thing and is
generally recommended (and shorter).
(I want to deprecate `group` and `groups`)
Tested: Refactoring.
CoreLibraryReviewExempt: Calling equivalent function.
Change-Id: I4c758968ae622fe16b7322be1b29b05b91e7fcd9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/489021
Reviewed-by: Paul Berry <paulberry@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Lasse Nielsen <lrn@google.com>
When running a large test file with `solo: true` on one test, the output looks like:
```
! test name
Skip: does not have "solo"
! test name
Skip: does not have "solo"
! test name
Skip: does not have "solo"
! test name
Skip: does not have "solo"
✓ solo test
! test name
Skip: does not have "solo"
```
While custom skip messages might be useful, when the skip messages are from using solo:true this just makes the output spammy and adds extra noise to scan through for the actual solo test you ran.
This change prevents creating output events for such skip messages (but still sends them for custom messages, since those are likely far less frequent).
The `print` events themselves are still forwarded to the DAP client, so it still has the data for this that it can present elsewhere (for example by showing the test node as skipped, if appropriate).
Fixes part of https://github.com/Dart-Code/Dart-Code/issues/5938
Change-Id: I1d26751207e7b89f77e13be0dfcc74710e7d75ec
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/486780
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Jessy Yameogo <yjessy@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
This test was flaky because thread-exit events are not guaranteed because the VM might terminate too quickly and we will instead emit a "terminated" event. Rather than trying to guarantee these events (for no benefit), this just makes the test accept the results with or without the text-exited event as long as we still got the terminated event.
Fixes https://github.com/dart-lang/sdk/issues/61826
Change-Id: I0dfbbae3f0d1f41f128ff19aa0f733aeba49f579
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/464680
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Jessy Yameogo <yjessy@google.com>
In order for "Copy Value" to work in VS Code's debugger windows, we need to provide an expression for each variable (the "evaluateName"). To carry this down through the child fields we need to store it by the object ID on the server, but we were not doing so for global fields - this resulted in "Copy Value" giving truncated values and "Copy Expression" being missing.
The fix is to ensure we call `storeEvaluateName` for globals too.
Fixes https://github.com/Dart-Code/Dart-Code/issues/5778
Change-Id: I9fed4b5483099341799548aea0225c9cef03d211
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/459560
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Jessy Yameogo <yjessy@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
A recent change somewhere caused some Dart-Code tests to fail. The expression "DateTime.now().ye" used to fail to compile and resulted in a failed RCP call to the VM service ("Expression compilation error") but now it returns a valid result that is a `vm.ErrorRef`.
This applies the same regex to the error message in this path to extract the most useful part of the error text when in the "watch" context so that the visible part of the message is not just "Unhandled exception".
Fixes https://github.com/Dart-Code/Dart-Code/issues/5759
Change-Id: I2933f9d811ec28446f4d2a857c1fd5bd18c94816
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/456241
Reviewed-by: Helin Shiah <helinx@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
Due to variable shadowing and lazy initialization of the DevTools URI
within DDS, the DevTools URI reported by DDS did not include the `uri`
query parameter used to automatically establish connection to the
current DDS instance.
This change updates variable names to avoid shadowing and also fixes
issues where the DevTools URI would be reported even if DevTools was not
enabled in the DDS configuration.
Change-Id: I1ab0f5c58ee7e583e6eb4f4fb7381cddc29629b1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/448480
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Auto-Submit: Ben Konyi <bkonyi@google.com>
This reverts two commits (in separate patch sets in the CL):
- commit 482a7caed7: [dap] Simplify URI handling in IsolateManager.addBreakpoint
- commit d0a7ef4459: [dds/dap] Add/remove breakpoints as required instead of replacing the whole set
The first revert is to avoid conflicts while reverting the second, and will be reapplied later (likely after the release branch, since it is not critical to include). The second revert is because this change resulted in leaked Script objects in the VM.
There is an additional change (patch set 3) to fix up the changelog/versions so they don't go backwards.
Change-Id: Id55949e1622d3367ced87c9a307c43881b030cbb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/440162
Reviewed-by: Slava Egorov <vegorov@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
- split the Dart CLI tool out of the VM into it's own embedder which
runs in AOT mode. The pure Dart VM executable is called 'dartvm' and
has no Dart CLI functionality in it
- the Dart CLI executable parses the CLI commands and invokes the rest
of the AOT tools in the same process, for the 'run' and 'test'
commands it execs a process which runs 'dartvm' to run
- 'dart hello.dart' execs the 'dartvm' process and runs 'hello.dart'
- the Dart CLI is not generated for ia32 as we are not shipping a
Dart SDK for ia32 anymore (support to execute the 'dartvm' for ia32
architecture is retained)
- the Dart CLI tool is not built in the internal Dart SDK builds
TEST=ci
Some performance improvement numbers
'dart format pkg/dartdev' goes from 1.17 secs to 0.22 secs
'dart doc pkg/dartdev' goes from 100.2 secs to 66.6 secs
'dart fix pkg/dartdev' goes from 19.3 secs to 14.5 secs
Change-Id: I66984a26cb2ab014b34dc1873f1f3d2884e13518
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/364202
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
The DAP APIs for breakpoints send the whole set of breakpoints for a given file at once. Previously, we would just delete all breakpoints and then re-add them all, however since we added support for resolving breakpoints, this can result in all breakpoints in a file flickering to unresolved then back to resolved (as well as generally being slower).
This change splits the method that would replace all breakpoints into methods for add+remove, and then skips over any breakpoints (in setBreakpoints) that already match breakpoints we have.
Fixes https://github.com/Dart-Code/Dart-Code/issues/4678
Change-Id: I489c6c295bac3ebd3a0851895a7a81e330d571e9
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/433700
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Derek Xu <derekx@google.com>
Commit-Queue: Derek Xu <derekx@google.com>
This is follow up to commit cb0c2bf5ed.
SampleBlockProcessor only enters isolate group and not a specific
isolate so the code must not rely on thread->isolate(). This fixes two
places where this was not the case:
* ProfileBuilder::IsPCInDartHeap
* UserTags::TagName
pkg/dds/test/get_cached_cpu_samples_test was supposed to cover this but
it has two problems:
First I observed that SampleBlockProcessor never gets a chance to
process a block if mutator thread always gets to it first (via a
scheduled interrupt), so this code is not well exercised. I started by
adding a variant of the test where interrupts are inhibited via a
vm:unsafe:no-interrupts pragma - which revealed the crashes in the
SampleBlockProcessor code.
This revealed the second problem: get_cached_cpu_samples_test does not
actually fail if testee crashes during the test, it just silently
completes with success. This seems to happen because disposal of
VmService connection is not forwarded into the future on which the test
is awaiting - and the whole process just exits once VmService connection
to the testee disappears (because all ports are closed, no pending
activity is possible after that one). I have fixed this by adding a
helper function which checks that connection to VmService only goes away
when we dispose it.
Note: there is another obvious issue here, which I am leaving unfixed
for now. SampleBlockProcessor calls UserTags::TagName in a way that can
race with isolate itself modifying the table. I think this race is
extremely unlikely but it can cause crashes on ARMs with its weak memory
model (e.g. we might end up reading garbage due to the reordering of
stores).
TEST=pkg/dds/test/get_cached_cpu_samples_test
Change-Id: Iee15ec2b019928b798c312e63edc76696abf5527
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/426300
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Slava Egorov <vegorov@google.com>
The change at https://dart-review.googlesource.com/c/sdk/+/410760 to fix a race incorrectly assumed that once we had handled startup for a thread, we would never need to send an automatic resume again. However this was not the case - after a hot reload, we need to resume the thread even though we had technically already handled startup.
This is essentially a partial revert of 84e6ed0784, with a test to verify we trigger readyToResume on PausePostRequest.
Change-Id: I1e171043f04f38dd6f52e1692d9257d34e5248be
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/416580
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Reviewed-by: Derek Xu <derekx@google.com>
The DAP tests have some debug logging that prints if a request/event takes longer than 10 seconds to arrive. In the case where tests run quickly and the DAP client is torn down, these delays cause the test run to stay alive for 10 sec unnecessarily (and print irrelevant warnings).
This changes that code to check periodically instead, and exits early if the client is torn down avoiding both the 10s wait and the spurious warning.
Change-Id: Ic07826942e8ae307867820020b30595a42e68d15
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/411680
Reviewed-by: Derek Xu <derekx@google.com>
Commit-Queue: Derek Xu <derekx@google.com>
Reviewed-by: Srujan Gaddam <srujzs@google.com>
While reviewing logs for flaky DAP tests, I found that these tests have been failing sometimes with errors that look like the isolate might not have paused on the `debugger()` call yet:
```
evaluateInFrame: (113) Expression compilation error
_buildExpressionEvaluationScope: invalid 'frameIndex' parameter: 0
```
and
```
evaluateInFrame: (113) Expression compilation error
org-dartlang-debug:synthetic_debug_expression:1:1: Error: Undefined name 'myInstance'.
myInstance
^^^^^^^^^^
```
This waits for the isolate to be paused, not just runnable, which I think might fix this.
Change-Id: Ib1d6b1c44a7ef93452065aa43c4d05058914db28
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/395302
Reviewed-by: Derek Xu <derekx@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
Ben Konyi