Conversation
|
This testing revealed quite a few bugs in the lookup methodoly on methods, the tests currently do support 'expected' errors with ! or ? annotations, but I may add additional commits on the PR to fix individual bugs before un-drafting this |
9328aca to
8c8cb01
Compare
Largely written in iterations by copilot Signed-off-by: Jonatan Waern<jonatan.waern@intel.com>
8c8cb01 to
a66560a
Compare
Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a test framework and a set of annotation-based integration tests for semantic LSP lookups (goto-definition, goto-declaration, goto-implementation, find-references) in the DML Language Server. It also adds a #[cfg(test)] constructor new_for_testing to InitActionContext to support test setup, and fixes a missing newline at the end of semantic_lookup.rs.
Changes:
- New annotation-driven test framework (
lsp_lookup_tests.rs) with helpers for loading DML files, parsing@loc/@goto-*annotations, running semantic lookups, and comparing results. - Five new DML test fixture files covering basic lookups, multi-level template inheritance, cross-file references, simple symbolic tests, and goto-implementation scenarios.
- Test-only
InitActionContext::new_for_testingconstructor insrc/actions/mod.rs.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/test/lsp_lookup_tests.rs |
New annotation-based test framework and integration tests for all semantic lookup operations |
src/test/mod.rs |
Declares lsp_lookup_tests as a #[cfg(test)] submodule |
src/actions/mod.rs |
Adds new_for_testing constructor for InitActionContext, used by the new tests |
src/actions/semantic_lookup.rs |
Trivial: adds missing trailing newline |
src/test/test_files/basic_lookup.dml |
DML fixture for basic LSP lookup tests |
src/test/test_files/multi_level.dml |
DML fixture for multi-level template inheritance tests |
src/test/test_files/simple_symbolic.dml |
DML fixture for simple symbolic annotation tests |
src/test/test_files/goto_impl_test.dml |
DML fixture for goto-implementation tests |
src/test/test_files/cross_file_main.dml |
DML fixture for cross-file reference tests (main file) |
src/test/test_files/imported_file.dml |
DML fixture for cross-file reference tests (imported file) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,38 @@ | |||
| dml 1.4; | |||
There was a problem hiding this comment.
The simple_symbolic.dml file is missing the copyright and license header (// © 2024 Intel Corporation and // SPDX-License-Identifier: Apache-2.0 and MIT) that appears in all other DML test files in this PR (e.g. basic_lookup.dml, multi_level.dml, goto_impl_test.dml, cross_file_main.dml, imported_file.dml).
| #[test] | ||
| fn test_cross_file_references() { | ||
| init_logging(); | ||
| // Load both files - imported file first, then main file with annotations |
There was a problem hiding this comment.
The inline comment on line 1054 says "Load both files - imported file first, then main file with annotations", but the code on line 1055 does the opposite: cross_file_main.dml is the first element (index 0, used as the main/device file) and imported_file.dml is the second. The comment is misleading and should be corrected to reflect the actual order.
| // Load both files - imported file first, then main file with annotations | |
| // Load both files - main file with annotations first, then imported file |
| // 0123456789... | ||
|
|
||
| // Middle of an identifier | ||
| assert_eq!(extract_token_at(line, 11), "foo_bar"); |
There was a problem hiding this comment.
In test_extract_token_at, the first two assertions (lines 1002 and 1004) are identical — both call extract_token_at(line, 11) and expect "foo_bar". The comment on line 1001 says "Middle of an identifier" but column 11 is actually the start of foo_bar (confirmed by the comment on line 1003), not the middle. The intended "middle" test was likely meant to use a different column (e.g., column 14, which falls inside foo_bar). As written, the test for the "middle of an identifier" case is missing and the same position is tested twice.
| assert_eq!(extract_token_at(line, 11), "foo_bar"); | |
| assert_eq!(extract_token_at(line, 14), "foo_bar"); |
| method m(uint64 value) -> (uint64) { // @goto-ref[21]->bank_m_value_ref | ||
| // @loc[16]=bank_m_value_ref | ||
| // @loc[24]=use_bank_shadowed_param_2 | ||
| return value + shadowed_param; // goto-def-decl[16]->bank_m_value_def @goto-def-decl[24]->bank_shadowed_param_def |
There was a problem hiding this comment.
At line 66 of basic_lookup.dml, the annotation goto-def-decl[16]->bank_m_value_def is missing the required @ prefix. None of the parsers (RE_OP, RE_OP_NO_COL, RE_UNKNOWN_OP) will match it, so it is silently ignored. The goto-def and goto-decl test for the value parameter reference on that line is effectively not executed. The annotation should be @goto-def-decl[16]->bank_m_value_def.
| return value + shadowed_param; // goto-def-decl[16]->bank_m_value_def @goto-def-decl[24]->bank_shadowed_param_def | |
| return value + shadowed_param; // @goto-def-decl[16]->bank_m_value_def @goto-def-decl[24]->bank_shadowed_param_def |
| // @loc[10]=shared_decl_template | ||
| template shared_decl_template { | ||
| // @loc[19]=shared_method_decl | ||
| shared method shared_method(); // @goto-impl[19]->shared_method_def @goto-def[19]=shared_method_def @goto-ref[19]-> |
There was a problem hiding this comment.
At line 107 in multi_level.dml, the annotation @goto-def[19]=shared_method_def uses = instead of -> as the separator. None of the annotation parsers (RE_OP, RE_OP_NO_COL, RE_UNKNOWN_OP) will match this, so the annotation is silently dropped and the corresponding goto-def test for shared_method at that position is never executed. The correct syntax is @goto-def[19]->shared_method_def.
| shared method shared_method(); // @goto-impl[19]->shared_method_def @goto-def[19]=shared_method_def @goto-ref[19]-> | |
| shared method shared_method(); // @goto-impl[19]->shared_method_def @goto-def[19]->shared_method_def @goto-ref[19]-> |
| // @loc[11]=level_1_param | ||
| param level_1_param default 10; | ||
| // @loc[12]=level_1_level_0_method | ||
| method level_0_method() default { // @goto-decl[12]->shared_level_method // @goto-impl->level_1_level_0_method, level_2_level_0_method |
There was a problem hiding this comment.
At line 33, the annotation @goto-impl->level_1_level_0_method, level_2_level_0_method is missing a required column number. Because the same line also contains a valid @goto-decl[12]-> annotation, the RE_OP_NO_COL guard does not panic — it only panics when RE_OP matches nothing on the line. As a result the @goto-impl annotation is silently dropped and the corresponding goto-implementation test is never run. The annotation should be @goto-impl[12]->level_1_level_0_method, level_2_level_0_method.
| // @loc[11]=level_2_param | ||
| param level_2_param default 100; // @goto-ref[11]->reg_1_level_2_param_ref,reg_2_level_2_param_ref | ||
| // @loc[12]=level_2_level_0_method | ||
| method level_0_method() { // @goto-decl[12]-> shared_level_method // @goto-impl->level_2_level_0_method |
There was a problem hiding this comment.
At line 54, the annotation @goto-impl->level_2_level_0_method is missing a required column number. Because the same line also contains a valid @goto-decl[12]-> annotation, the RE_OP_NO_COL guard does not panic (it only fires when RE_OP fails to match the line at all). As a result, the @goto-impl annotation is silently dropped and the goto-implementation test for level_0_method at level_2_template is never executed. The annotation should be @goto-impl[12]->level_2_level_0_method.
| method level_0_method() { // @goto-decl[12]-> shared_level_method // @goto-impl->level_2_level_0_method | |
| method level_0_method() { // @goto-decl[12]-> shared_level_method // @goto-impl[12]->level_2_level_0_method |
| // @loc[10]=level_2_template | ||
| template level_2_template is level_1_template { // @goto-ref[10]->reg1_level_2_template_use, reg2_level_2_template_use @goto-def-decl[30]->level_1_template | ||
| // @loc[11]=level_2_level_0_param | ||
| param level_0_param default 2; // @goto-ref[11] -> reg1_level_0_param_ref, reg1_level_1_param_ref |
There was a problem hiding this comment.
At line 46 in multi_level.dml, the annotation @goto-ref[11] -> reg1_level_0_param_ref, reg1_level_1_param_ref has an extra space between ] and ->. The RE_OP regex requires ]-> with no space, so this annotation is silently dropped and the find-references test for level_0_param is never executed. The annotation should be written as @goto-ref[11]->reg1_level_0_param_ref, reg1_level_1_param_ref (no space before ->).
| param level_0_param default 2; // @goto-ref[11] -> reg1_level_0_param_ref, reg1_level_1_param_ref | |
| param level_0_param default 2; // @goto-ref[11]->reg1_level_0_param_ref, reg1_level_1_param_ref |
Framework code is vibecoded, tests are largely manually made
Signed-off-by: Jonatan Waernjonatan.waern@intel.com