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

[K9VULN-1925] fix: improve unsound test by testing the production code path #587

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

amaanq
Copy link
Collaborator

@amaanq amaanq commented Dec 19, 2024

What problem are you trying to solve?

In #555 we added support for timing out tree-sitter query executions, as this was necessary to prevent the analyzing spinning forever in case a query suffered from combinatorial explosion. However, with that change, the test added to verify this actually works used the shorthand_execute_rule_internal function, when it should have used shorthand_execute_rule instead because the former does not test the production code path, so any changes to JsRuntime::execute_rule that affect the query timeout will not be caught by the test, unless the corresponding internal function is updated as well.

What is your solution?

Instead of using shorthand_execute_rule_internal, we should be using shorthand_execute_rule to test the production code path, which this PR does.

Alternatives considered

What the reviewer should know

shorthand_execute_rule takes in an optional ExecuteOptions struct. All tests that use this function, though, pass in None for this argument, and as such, never suffered from the fact that the fields of this struct are private, and there's no method on this struct to create an instance of it. As such, I've marked the fields as pub(crate) so that I can create this struct with a timeout and pass it into shorthand_execute_rule.

@amaanq amaanq requested a review from a team as a code owner December 19, 2024 20:47
@amaanq amaanq requested review from dastrong and jasonforal and removed request for dastrong December 19, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant