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.

Sign up for GitHub

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

Jump to bottom

Dynamically detected test step should be removed when a test run omits it #900

Open
nayeemrmn opened this issue Aug 30, 2023 · 0 comments
Open

Dynamically detected test step should be removed when a test run omits it #900

nayeemrmn opened this issue Aug 30, 2023 · 0 comments
Assignees
@nayeemrmn
Labels
enhancement New feature or request testing Running test using LSP

Comments

@nayeemrmn
Copy link
Collaborator

Dynamically detected test steps are preserved in the test explorer on file change because they are normally still there. However, we should try to determine when they are actually deleted like if a test run happens and it appears to be gone.
@nayeemrmn nayeemrmn added enhancement New feature or request testing Running test using LSP labels Aug 30, 2023
@nayeemrmn nayeemrmn mentioned this issue Aug 30, 2023
Merged
nayeemrmn added a commit to denoland/deno that referenced this issue Aug 30, 2023
@nayeemrmn
refactor(lsp): store test definitions in adjacency list (#20330)
d28384c 
Previously:
```rust
pub struct TestDefinition {
  pub id: String,
  pub name: String,
  pub range: SourceRange,
  pub steps: Vec<TestDefinition>,
}

pub struct TestDefinitions {
  pub discovered: Vec<TestDefinition>,
  pub injected: Vec<lsp_custom::TestData>,
  pub script_version: String,
}
```
Now:
```rust
pub struct TestDefinition {
  pub id: String,
  pub name: String,
  pub range: Option<Range>,
  pub is_dynamic: bool, // True for 'injected' module, not statically detected but added at runtime.
  pub parent_id: Option<String>,
  pub step_ids: HashSet<String>,
}

pub struct TestModule {
  pub specifier: ModuleSpecifier,
  pub script_version: String,
  pub defs: HashMap<String, TestDefinition>,
}
```

Storing the test tree as a literal tree diminishes the value of IDs,
even though vscode stores them that way. This makes all data easily
accessible from `TestModule`. It unifies the interface between
'discovered' and 'injected' tests. This unblocks some enhancements wrt
syncing tests between the LSP and extension, such as this TODO:
https://github.com/denoland/vscode_deno/blob/61f08d5a71536a0a5f7dce965955b09e6bd957e1/client/src/testing.ts#L251-L259
and denoland/vscode_deno#900. We should also
get more flexibility overall.

`TestCollector` is cleaned up, now stores a `&mut TestModule` directly
and registers tests as it comes across them with
`TestModule::register()`. This method ensures sanity in the redundant
data from having both of `TestDefinition::{parent_id,step_ids}`.

All of the messy conversions between `TestDescription`,
`LspTestDescription`, `TestDefinition`, `TestData` and `TestIdentifier`
are cleaned up. They shouldn't have been using `impl From` and now the
full list of tests is available to their implementations.
@nayeemrmn nayeemrmn self-assigned this Aug 30, 2023
bartlomieju pushed a commit to denoland/deno that referenced this issue Aug 31, 2023
@nayeemrmn @bartlomieju
refactor(lsp): store test definitions in adjacency list (#20330)
d592cf0 
Previously:
```rust
pub struct TestDefinition {
  pub id: String,
  pub name: String,
  pub range: SourceRange,
  pub steps: Vec<TestDefinition>,
}

pub struct TestDefinitions {
  pub discovered: Vec<TestDefinition>,
  pub injected: Vec<lsp_custom::TestData>,
  pub script_version: String,
}
```
Now:
```rust
pub struct TestDefinition {
  pub id: String,
  pub name: String,
  pub range: Option<Range>,
  pub is_dynamic: bool, // True for 'injected' module, not statically detected but added at runtime.
  pub parent_id: Option<String>,
  pub step_ids: HashSet<String>,
}

pub struct TestModule {
  pub specifier: ModuleSpecifier,
  pub script_version: String,
  pub defs: HashMap<String, TestDefinition>,
}
```

Storing the test tree as a literal tree diminishes the value of IDs,
even though vscode stores them that way. This makes all data easily
accessible from `TestModule`. It unifies the interface between
'discovered' and 'injected' tests. This unblocks some enhancements wrt
syncing tests between the LSP and extension, such as this TODO:
https://github.com/denoland/vscode_deno/blob/61f08d5a71536a0a5f7dce965955b09e6bd957e1/client/src/testing.ts#L251-L259
and denoland/vscode_deno#900. We should also
get more flexibility overall.

`TestCollector` is cleaned up, now stores a `&mut TestModule` directly
and registers tests as it comes across them with
`TestModule::register()`. This method ensures sanity in the redundant
data from having both of `TestDefinition::{parent_id,step_ids}`.

All of the messy conversions between `TestDescription`,
`LspTestDescription`, `TestDefinition`, `TestData` and `TestIdentifier`
are cleaned up. They shouldn't have been using `impl From` and now the
full list of tests is available to their implementations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nayeemrmn nayeemrmn

Labels
enhancement New feature or request testing Running test using LSP
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@nayeemrmn