-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
refactor: Allocate IDs for tests #14729
Conversation
state: &mut OpState, | ||
info: TestStepInfo, | ||
) -> Result<TestRegisterResult, AnyError> { | ||
let id = NEXT_ID.fetch_add(1, Ordering::SeqCst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens in the vscode's testing sidebar if you run a test with steps multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Probably @kitsonk should review the LSP changes before merge though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work Nayeem. I verified that LSP changes work properly in vscode_deno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned some of the changes to the lsp aren't "cleanup" but arbitrary changes. Specifically for the messages that go to and from the client that are changed should have their types changed in https://github.com/denoland/vscode_deno/blob/main/client/src/lsp_extensions.ts
to ensure there isn't a knock on effect. Especially when we remove Option
it becomes a mandatory field on deserializing.
@@ -53,6 +54,6 @@ bar 2 => ./test/uncaught_errors_2.ts:3:6 | |||
bar 3 => ./test/uncaught_errors_2.ts:6:6 | |||
./test/uncaught_errors_3.ts (uncaught error) | |||
|
|||
FAILED | 2 passed | 5 failed ([WILDCARD]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nayeemrmn seems like this is flaky now: https://github.com/denoland/deno/runs/7362087847?check_suite_focus=true
This reverts commit 22a4998.
Blocked on #14757.
Allows for better state-keeping of tests in Rust:
... cancelled
for tests that were registered and cancelled due to uncaught errors:someTest ...
when a result arrives (addresses Deno.test with output: Show ok on the same line as the test name #14315 (comment)).Adds
op_register_test()
to register tests, allocate IDs to them and potentially other info:{ id, filteredOut }
is returned by this op.... cancelled
output above for tests that haven't started yet.Cleanups in LSP test runner and more functional approach in
40_testing.js
. Bring bench runner error formatting up to date, it was still usingformatError()
in JS.