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

refactor: Allocate IDs for tests #14729

Merged
merged 20 commits into from
Jul 15, 2022
Merged

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented May 26, 2022

Blocked on #14757.

Allows for better state-keeping of tests in Rust:

  • Prints ... cancelled for tests that were registered and cancelled due to uncaught errors:
    running 3 tests from ./temp.ts
    testWithUncaughtRejection ...
    Uncaught error from ./temp.ts FAILED
    testWithUncaughtRejection ... cancelled (0ms)
    anotherTest ... cancelled (0ms)
    anotherOne ... cancelled (0ms)
    
  • Always know whether or not to reprint someTest ... when a result arrives (addresses Deno.test with output: Show ok on the same line as the test name #14315 (comment)).
  • Allows lots of future improvements to output e.g. more self-contained result lines for concurrent runs:
    (./temp.ts) t1 ... ok
    (./temp.ts) t2 ... t2-step1 ... FAILED
    (./temp.ts) t2 ... t2-step2 ... ok
    (./temp.ts) t2 ... FAILED
    

Adds op_register_test() to register tests, allocate IDs to them and potentially other info:

  • Move filtering to rust, { id, filteredOut } is returned by this op.
  • Tests are known about in Rust earlier, allowing for the ... 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 using formatError() in JS.

@nayeemrmn nayeemrmn changed the title refactor: Allocate IDs for tests and benches refactor: Allocate IDs for tests May 30, 2022
state: &mut OpState,
info: TestStepInfo,
) -> Result<TestRegisterResult, AnyError> {
let id = NEXT_ID.fetch_add(1, Ordering::SeqCst);
Copy link
Member

@dsherret dsherret May 31, 2022

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?

Copy link
Member

Choose a reason for hiding this comment

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

Seems to work.

Copy link
Member

@dsherret dsherret left a 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.

Copy link
Member

@bartlomieju bartlomieju left a 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.

Copy link
Contributor

@kitsonk kitsonk left a 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.

cli/lsp/testing/collectors.rs Show resolved Hide resolved
cli/lsp/testing/collectors.rs Show resolved Hide resolved
cli/lsp/testing/definitions.rs Show resolved Hide resolved
cli/lsp/testing/execution.rs Show resolved Hide resolved
cli/lsp/testing/execution.rs Show resolved Hide resolved
cli/lsp/testing/execution.rs Show resolved Hide resolved
cli/lsp/testing/execution.rs Show resolved Hide resolved
cli/lsp/testing/execution.rs Show resolved Hide resolved
cli/lsp/testing/lsp_custom.rs Show resolved Hide resolved
cli/lsp/testing/lsp_custom.rs Show resolved Hide resolved
@dsherret dsherret added this to the 1.24 milestone Jul 12, 2022
@dsherret dsherret merged commit 22a4998 into denoland:main Jul 15, 2022
@@ -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])
Copy link
Member

Choose a reason for hiding this comment

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

@nayeemrmn nayeemrmn deleted the id-tests-and-benches branch July 15, 2022 18:42
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Jul 18, 2022
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.

None yet

4 participants