-
Notifications
You must be signed in to change notification settings - Fork 68
Add JSON schema for test runner arguments #169
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
Merged
Merged
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
10e3cfe
Add JSON schema for test runner arguments
natebosch 60648ea
Use a constant in ParameterNames
natebosch 44674b2
Add blocklist
natebosch d092311
Remove extra escapes
natebosch 8b6e88d
Add comment with update instructions
natebosch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why is this a String, how are we intended to update it?
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 also not sure many of these make sense to expose and are just a waste of tokens and will make overall calls less reliable?
For instance
color,help,file-reporter,pause-after-load, etc?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.
We'd dump it from the test runner binary again and replace the entire String. We can change this away from a String, it will add steps to update, but I don't think that's a problem. Would you prefer a map literal?
I don't think it makes sense to curate the whole list manually, but defining a blocklist is feasible. I'd worry about forgetting to add an argument sometime and leaving a gap where the client needs to use the shell even though we said not to. WDYT about a blocklist defined here? If a blocklist defined by the test runner is better we'd need to update the
package:argsAPI.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 think
file-reporterhas plausible utility for a client, I added a blocklist with the others.For the overall token usage I'm partially running on a hunch that some sort of tool filtering is likely to become a standard feature for clients. There is a huge number of tools being created and a number of discussions started about managing the volume of tools. I started exploring tool filtering by making a call with less historical context to decide which tools should offered but I didn't figure out how to evaluate the behavior impacts.
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.
Blocklist sounds fine to me
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 added instructions for updating this. It currently relies on unmerged PRs in
package:argsandpackage:test. I'd be fine seeing these land if someone wants to tackle it before I'm back, otherwise I think it's OK if these sit as drafts just in case they are needed and I'll revisit whether it's worth landing in a few weeks.dart-lang/test#2508