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

Support post-procesed inline expectations for query predicates in unit tests #19211

Merged
merged 2 commits into from
Apr 3, 2025

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Apr 3, 2025

This adds a low-friction way to add inline expectations to a unit test.

Any .ql file in a test folder can now use inline expectations without doing the dance of importing and instantiating a parameterised module.

Example

Suppose we have a file test.ql with this content:

import javascript
import semmle.javascript.security.SensitiveActions

query predicate sensitiveExpr(SensitiveNode e, string kind) { kind = e.getClassification() }

query predicate processTermination(NodeJSLib::ProcessTermination term) { any() }

We can now add a test.qlref in the same folder with the contents:

query: tests.ql
postprocess: utils/test/InlineExpectationsTestQuery.ql

And now the test will run with inline expectation checking. For example the test file might look like this:

x.get("password"); // $ sensitiveExpr=password
process.exit(); // $ processTermination

Details

  • The name of a query predicate becomes the tag.
  • The first column, which should be an entity type, becomes the location.
  • The second column, which should be a primitive type like string, becomes the value. This column may be omitted.
  • Predicates with the wrong number of columns are simply ignored.
  • The test.ql file must have no query kind for this to take effect. Queries of kind problem or path-problem are already supported in their own way.

This PR ports a single test to use this in order to keep the PR small while ensuring the behaviour is exercised.

Thanks to @hmakholm for some corresponding fixes in the CLI, to ensure the same-named .ql and .qlref files can coexist in the same folder without fighting over the same .actual and .expected file.

@github-actions github-actions bot added the JS label Apr 3, 2025
@asgerf asgerf marked this pull request as ready for review April 3, 2025 12:08
@Copilot Copilot bot review requested due to automatic review settings April 3, 2025 12:08
@asgerf asgerf requested a review from a team as a code owner April 3, 2025 12:08
@asgerf asgerf added the no-change-note-required This PR does not need a change note label Apr 3, 2025
@asgerf asgerf requested a review from hvitved April 3, 2025 12:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for inline expectations for query predicates in unit tests, providing a convenient way to specify expected values directly in test files.

  • Updated test cases to include inline expectation annotations.
  • Added inline annotations for sensitive expressions and process termination.
Files not reviewed (4)
  • javascript/ql/test/library-tests/SensitiveActions/tests.expected: Language not supported
  • javascript/ql/test/library-tests/SensitiveActions/tests.ql: Language not supported
  • javascript/ql/test/library-tests/SensitiveActions/tests.qlref: Language not supported
  • shared/util/codeql/util/test/InlineExpectationsTest.qll: Language not supported
Comments suppressed due to low confidence (2)

javascript/ql/test/library-tests/SensitiveActions/tst.js:1

  • [nitpick] The inline expectation uses 'cleartextPasswordExpr' as the location while the predicate is 'sensitiveExpr'. Consider aligning the naming for consistency so that the entity type clearly reflects the expected predicate.
password; // $ cleartextPasswordExpr sensitiveExpr=password

javascript/ql/test/library-tests/SensitiveActions/tst.js:18

  • [nitpick] The inline expectation comment for process termination includes a location 'processTermination' with a value of 'sensitiveAction'. Verify that this naming reflects the intended query predicate and entity type, and adjust if necessary for clarity and consistency.
e(); // $ processTermination sensitiveAction

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

@hvitved
Copy link
Contributor

hvitved commented Apr 3, 2025

Thanks to @hmakholm for some corresponding fixes in the CLI, to ensure the same-named .ql and .qlref files can coexist in the same folder without fighting over the same .actual and .expected file.

Which .expected file does the .qlref file then use?

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@asgerf
Copy link
Contributor Author

asgerf commented Apr 3, 2025

Which .expected file does the .qlref file then use?

See the backlinked PR. Only the .qlref file is scheduled as a test, so the .expected file belongs to the .qlref file. But as along as there are no test failures, it should be identical to that of the .ql file.

@asgerf asgerf merged commit 68f6f9f into github:main Apr 3, 2025
37 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants