-
Notifications
You must be signed in to change notification settings - Fork 98
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
Test Explorer in VS Code Extension and @Test()
Attribute
#2059
base: main
Are you sure you want to change the base?
Conversation
This was a drive-by update from my work in #2059. Just wanted it in a separate PR to keep the noise down. It seems to have triggered some `prettier` fixes too.
if !callable.generics.is_empty() | ||
|| callable.input.kind != PatKind::Tuple(vec![]) | ||
{ | ||
return None; |
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.
So does this just silently drop a callable with the Test attribute if it happens to be generic? No warning or error?
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.
The error is detected in qsc_passes
. Returning an error here doesn't show a pretty error to the user, so instead we detect it in passes and produce a spanned error. This pattern was suggested by @swernli and I think it is cleaner than returning an error here, which just prints an unspanned error in a VS Code dialog box.
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.
Yeah I saw those checks there. Just wondering the value of even testing for it here also, as the project is already in error from the other pass that detects these, and thus I assume shouldn't run anyway.
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.
It just prevents the test from showing up in the list. The pass doesn't filter out the callables from the HIR, because we prefer to preserve as much of the AST as possible for error recovery in passes. So even in an error state, the compilation does return some HIR for a good user experience. We can't let those invalid callables show up in the test explorer right?
const program = programResult.programConfig; | ||
|
||
for (const testCase of request.include || []) { | ||
await runTestCase(ctrl, testCase, request, worker, program); |
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 not sure I follow the use of the "common worker" here. I thought we discussed the language service could return the list of tests (as this has already parsed/compiled the project on every update and is thus close to zero cost), but the tests should run in an isolated worker for reliability.
Here is seems we're using a common worker to find all the tests (thus on every 'update document' we're compiling the project twice, once for the language service and once for the test discovery - even though most projects will have no tests), and are also running all the tests in that same common worker - not isolated.
Or am I reading this wrong?
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 see what you're saying now, thanks for typing it out. After our in-person discussion, I had it in my mind that I needed to a) mitigate the worker leak and b) reuse the worker from the language service. I had those two goals in mind when I wrote that code.
I like the approach of exposing a specific "get test callables" function in the compiler API, because it allows for other scenarios to consume it and write testing infrastructure. Integrating that functionality into the language service to be a language-service-exclusive feature sounds like signing ourselves up for more work in the future.
I think a good approach here would be to modify compile
such that the returned asset (a CompileUnit
) contains the list of test callables, and remove getTestCallables()
elsewhere. Thoughts?
This PR is for enabling VS Code's Test Explorer functionality in our extension, and it introduces the
@Test()
attribute on callables. It also switches our libraries over to use@Test()
instead of the oldMain
-based pattern.Any callable that takes no arguments can be annotated as a
@Test()
. If it does not crash (viaFact
or otherwise), it is considered a pass. If it does crash, then it is considered a failure.After this PR is in, when you open a .qs file, you'll see this icon:
Clicking this icon will take you to the test explorer:
The test explorer shows all discovered tests in their namespace hierarchy. Clicking the "run" play button on a namespace runs all child tests contained in that namespace. A test turns green if its last run passed, or red if it didn't.
Other features:
"Run Test" button next to the test itself
"Failed test" icon on failed tests
Notes:
[object Object]
as the output of a failed run.updateDocument
is triggered. The test explorer listens for this event to refresh tests for a specific URI. The test discovery code has to scan for tests across an entire package, since tests could be added to a namespace from any file due to explicit namespaces. But only the updates from the new URI are actually populated to the test explorer.Potential future work:
@Test()
(this was deferred for now as I want that experience to integrate with the Debugger service and that requires a bit more work).