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

scip test command #235

Closed
matthewnitschke opened this issue Apr 6, 2024 · 4 comments · Fixed by #236
Closed

scip test command #235

matthewnitschke opened this issue Apr 6, 2024 · 4 comments · Fixed by #236

Comments

@matthewnitschke
Copy link

While developing tree-sitter grammars, I've gotten pretty used to the tree-sitter test command, where selections are explicitly stated within test files, and the result passes and fails based on whether the parser matches whats specified

What would the thought be around creating a scip test command which mirrors how tree-sitter highlight tests work?

For example, the following file would be committed a test/ directory, and scip test ./index.scip would pass or fail depending on if the Foo symbol had the correct definition

class Foo {
//    ^ definition scip-dart pub some_package 1.0.0 lib/`main.dart`/main().
}

Ideally all "selectors" could be used this way as well, reference, documentation, diagnostic, and relationship...

This would be offered as an alternate to the existing scip snapshot, where individual language features are tested, making it clearer from a test file perspective what is intentional

Copy link
Contributor

Not opposed to adding something here if it helps simplify something downstream, but I'm afraid I don't quite understand the distinction you're making with "making it clearer from a test file perspective what is intentional". AFAICT, the tree-sitter test sub-command also checks the full tree, similar to how the snapshot sub-command generates a full snapshot.

Could you elaborate on the expected difference between running scip snapshot + a separate diff command compared to the proposed test sub-command?

@matthewnitschke
Copy link
Author

matthewnitschke commented Apr 9, 2024

👋 Hey @varungandhi-src

AFAICT, the tree-sitter test sub-command also checks the full tree, similar to how the snapshot sub-command generates a full snapshot.

I think I was specifically referring to the highlight query tests as opposed to the corpus tests

scip snapshot is great for testing a large amount of source files without much manual effort. But if I want to hyper focus on a specific piece of indexing functionality, I'd prefer my file to be explicitly written by an dev in the project, and only stating the symbols which I want to validate

For a more concrete example, say I want to specifically test how scip-dart handles function indexing, if I were to test this using the existing scip snapshot, the generated file would look like the following

  void foo(String bar) {
// definition scip-dart pub dart_test 1.0.0 lib/`main.dart`/
//     ^^^ definition scip-dart pub dart_test 1.0.0 lib/`main.dart`/foo().
//         ^^^^^^ reference scip-dart pub dart:core 2.19.0 dart:core/`string.dart`/String#
//                ^^^ definition local 0
    print('test');
//  ^^^^^ reference scip-dart pub dart:core 2.19.0 dart:core/`print.dart`/print().
  }

This is a lot of data, and obscures the specifics of what I'd like to test. If instead I could specifically write the "symbols" which I'd like to test, this could be written as follows:

void foo(String bar) {
  // ^ definition scip-dart pub dart_test 1.0.0 lib/`main.dart`/foo().
  print('test');
  // <-  reference scip-dart pub dart:core 2.19.0 dart:core/`print.dart`/print().
}

This is much clearer that I'm specifically trying to validate functions and their invocations, and is an explicit "pass/fail" determined by an explicit dev making a written assumption

Copy link
Contributor

Understood. I think this would be OK to add as a separate sub-command, I can see the usefulness. We don't have bandwidth to tackle this ourselves right now, but I can review a PR if someone is willing to implement it.

@matthewnitschke
Copy link
Author

Yep! No worries, that was my assumption

Just wanted to verify it would be an acceptable PR before doing the implementation work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants