-
Notifications
You must be signed in to change notification settings - Fork 219
Allow running tests by line and column #1604
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
Conversation
- **full-name**: Requires an match for the name of the test. | ||
- **line**: Matches any test that originates from this line in the test suite. | ||
- Not supported for browser tests. | ||
- **col**: Matches any test that originates from this column in the test suite. |
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 confused who would use this? Is this a specific column? Like the position of the first t
in test(
?
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.
Yes - only IDEs would use it most likely
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 reason for it is protection against accidentally running tests the user doesn't expect if they have duplicate names:
// Click "Run" next to this test currently runs both tests, which seems odd to the user
test('foo', () {});
test('foo', () {});
Although the IDE will need to be sure of the package:test
version before trying to use these new features.
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.
Ya and having the col
option specifically guards against having two tests on the same line. A rare situation but its trivial to support once we are already supporting filtering by line.
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 still don't understand the precise position this matches.
Is it exactly the first t
in test
? Is it 0 based? Does this match the same frame as the line number, or can the line match one frame and the column match a different frame?
I'll read through the test cases tomorrow to find out more, but I do think we should expand the user facing docs.
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 matches the column position of the function invocation in the stack trace. That is the position of the first character of the name of the function (so if your cursor is to the left of it). It is one based, which matches what the IDE reports (or at least VsCode, I assume this is standard?).
I'll read through the test cases tomorrow to find out more, but I do think we should expand the user facing docs.
There is a sentence after these that says "all filters must match for a test to be ran". I could move it to the top?
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.
Ok I added a mini section under this describing the specific line/col semantics, and also moved the note about all filters matching up to the first paragraph.
I presume this is to support things like eg. if I pass (although, maybe that wouldn't be a big deal if it's encouraged to always pass a name, even when using line/col). |
Yes exactly, it essentially allows for "parameterized tests". Probably the IDE doesn't have enough info to know about these actual call sites though (the "root_line" etc would be the actual
Yes it must match the test suite path. |
Great, thanks! :-) |
…m if a formatter is available
Added support and a test for browser/node tests, turned out to be pretty easy :) |
- **full-name**: Requires an match for the name of the test. | ||
- **line**: Matches any test that originates from this line in the test suite. | ||
- Not supported for browser tests. | ||
- **col**: Matches any test that originates from this column in the test suite. |
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 still don't understand the precise position this matches.
Is it exactly the first t
in test
? Is it 0 based? Does this match the same frame as the line number, or can the line match one frame and the column match a different frame?
I'll read through the test cases tomorrow to find out more, but I do think we should expand the user facing docs.
Co-authored-by: Nate Bosch <[email protected]>
pkgs/test_core/lib/src/runner.dart
Outdated
var line = suite.config.line; | ||
var col = suite.config.col; | ||
var trace = test.trace; | ||
if (trace == null && (line != null || col != null)) { |
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.
When would we be missing a trace? Should we consider it an error to try to run tests with a line and column when we have no trace available?
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 am not sure if we get load tests and things through this api. I will try making it an error and see what happens.
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.
For now I am just throwing a StateError, let me know if you think a different one would be more appropriate.
Fixes #1579
This does not currently work on browsers, but I probably could get it to? Not sure how important that ultimately is.trace
sent back for groups/tests that are discovered (enabling browser/node support).cc @DanTup