-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Shared: Post-processing query for inline test expectations #17548
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
Conversation
9c53960 to
3daa0e3
Compare
bc0a80e to
b689ace
Compare
0d38edf to
1e71fe4
Compare
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.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
3574391 to
d733473
Compare
9a59207 to
dd520fe
Compare
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 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.
geoffw0
left a comment
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.
Minor corrections for Swift. It's really good to have this and we would benefit from using it more widely!
jketema
left a comment
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.
Some small comments (C++ and Swift)
cpp/ql/test/query-tests/Security/CWE/CWE-022/semmle/tests/test.c
Outdated
Show resolved
Hide resolved
geoffw0
left a comment
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.
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?
Co-authored-by: Geoffrey White <40627776+geoffw0@users.noreply.github.com>
No; I'll add that. |
|
@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. |
0657163 to
495c92d
Compare
michaelnebel
left a comment
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.
C# LGTM 👍
geoffw0
left a comment
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.
LGTM too. 👍
tausbn
left a comment
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.
Python looks good to me. 👍
This PR introduces support for writing (and verifying) inline test expectations for
.qlreffiles, for all languages. The implementation makes use of a@kind test-postprocessquery, which must be referenced in the relevant.qlreffiles.Syntax
The postprocessing query works for queries of kind
problemandpath-problem, and each query result must have a matching$ Alertcomment. It is possible to augment the comment with a query ID, in order to support cases where multiple.qlreftests share the same test code:In the example above, the
$ Alert[rust/unused-value]commment is only taken into account in the test for the query with IDrust/unused-value, and vice versa for the$ Alert[rust/unreachable-code]comment.For
path-problemqueries, each source and sink must additionally be annotated ($ Sourceand$ Sink, respectively), except when their location coincides with the location of the alert itself, in which case only$ Alertis needed.Example:
Morover, it is possible to tag sources with a unique identifier:
In this case, the source and sink must have the same tag in order to be matched.
Output
After enabling the
@kind test-postprocessquery in a.qlreftest, the.expectedfile will contain failures in atestFailuresoutput relation, similar to when inline test expectations are used in library tests. Unlike library tests, however, thetestFailurespredicate will only be present in the.expectedfile when it is non-empty.Actual query output, such as
#selectandedges, will still be present in the.expectedfile, which is deliberate. For data flow queries in particular, having theedgesrelation explicit in the output has proven very useful in the past for identifying unintended data flow changes (this is why the sharedInlineFlowTestlibrary also includes the data flow graph, see for example this library test).