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

Shared: Post-processing query for inline test expectations #17548

Merged
merged 15 commits into from
Oct 31, 2024

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Sep 23, 2024

This PR introduces support for writing (and verifying) inline test expectations for .qlref files, for all languages. The implementation makes use of a @kind test-postprocess query, which must be referenced in the relevant .qlref files.

Syntax

The postprocessing query works for queries of kind problem and path-problem, and each query result must have a matching $ Alert comment. It is possible to augment the comment with a query ID, in order to support cases where multiple .qlref tests share the same test code:

var x = ""; // $ Alert[rust/unused-value]
return;
foo();      // $ Alert[rust/unreachable-code]

In the example above, the $ Alert[rust/unused-value] commment is only taken into account in the test for the query with ID rust/unused-value, and vice versa for the $ Alert[rust/unreachable-code] comment.

For path-problem queries, each source and sink must additionally be annotated ($ Source and $ Sink, respectively), except when their location coincides with the location of the alert itself, in which case only $ Alert is needed.

Example:

var queryParam = Request.QueryString["param"]; // $ Source
Write(Html.Raw(queryParam));                   // $ Alert

Morover, it is possible to tag sources with a unique identifier:

var queryParam = Request.QueryString["param"]; // $ Source=source1
Write(Html.Raw(queryParam));                   // $ Alert=source1

In this case, the source and sink must have the same tag in order to be matched.

Output

After enabling the @kind test-postprocess query in a .qlref test, the .expected file will contain failures in a testFailures output relation, similar to when inline test expectations are used in library tests. Unlike library tests, however, the testFailures predicate will only be present in the .expected file when it is non-empty.

Actual query output, such as #select and edges, will still be present in the .expected file, which is deliberate. For data flow queries in particular, having the edges relation explicit in the output has proven very useful in the past for identifying unintended data flow changes (this is why the shared InlineFlowTest library also includes the data flow graph, see for example this library test).

@hvitved hvitved force-pushed the shared/inline-test-post-process branch 3 times, most recently from 9c53960 to 3daa0e3 Compare October 7, 2024 11:12
@github-actions github-actions bot added the Rust Pull requests that update Rust code label Oct 7, 2024
@hvitved hvitved force-pushed the shared/inline-test-post-process branch 2 times, most recently from bc0a80e to b689ace Compare October 7, 2024 11:57
@hvitved hvitved force-pushed the shared/inline-test-post-process branch 10 times, most recently from 0d38edf to 1e71fe4 Compare October 28, 2024 18:26
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@hvitved hvitved force-pushed the shared/inline-test-post-process branch 3 times, most recently from 3574391 to d733473 Compare October 29, 2024 09:25
@hvitved hvitved force-pushed the shared/inline-test-post-process branch from 9a59207 to dd520fe Compare October 29, 2024 12:37
owen-mc
owen-mc previously approved these changes Oct 29, 2024
Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

The Go parts LGTM. If I have time before this is merged I will try to start converting more tests to use it, which would be a better test.

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Minor corrections for Swift. It's really good to have this and we would benefit from using it more widely!

Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

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

Some small comments (C++ and Swift)

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

Minor comments for Rust as well.

Do we have any way to know for sure that [query] inline expectations will fail when an annotation is missing or incorrect? In other words, is there a test for inline expectations tests themselves?

@hvitved
Copy link
Contributor Author

hvitved commented Oct 29, 2024

Do we have any way to know for sure that [query] inline expectations will fail when an annotation is missing or incorrect? In other words, is there a test for inline expectations tests themselves?

No; I'll add that.

@hvitved
Copy link
Contributor Author

hvitved commented Oct 30, 2024

@geoffw0 : Thanks for the suggestion to add actual tests of the library, which I have now added (in C#). This revealed a bug when using source/sink tags in conjunction with query ID tags, which has been fixed.

@hvitved hvitved force-pushed the shared/inline-test-post-process branch from 0657163 to 495c92d Compare October 30, 2024 09:55
Copy link
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

C# LGTM 👍

Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM too. 👍

Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Python looks good to me. 👍

@hvitved hvitved merged commit 2b37c6c into github:main Oct 31, 2024
62 checks passed
@hvitved hvitved deleted the shared/inline-test-post-process branch October 31, 2024 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# C++ DataFlow Library Go Java JS no-change-note-required This PR does not need a change note Python Ruby Rust Pull requests that update Rust code Swift
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants