Skip to content

Commit c8ef2de

Browse files
Update .github/copilot-instructions.md
Co-authored-by: Michael R Fairhurst <MichaelRFairhurst@github.com>
1 parent 83da8b3 commit c8ef2de

File tree

1 file changed

+31
-0
lines changed

1 file changed

+31
-0
lines changed

.github/copilot-instructions.md

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,3 +26,34 @@ For PRs that add new queries or modify existing queries, also consider the follo
2626
- Can the query be simplified further (not golfed!).
2727

2828
In your review output, list only those checklist items that are not satisfied or are uncertain, but also report any other problems you find outside this checklist; do not mention checklist items that clearly pass.
29+
30+
## Validating tests and .expected files
31+
32+
The test infrastructure for CodeQL that we use in this project involves the creation of a test directory with the following structure:
33+
- Test root is `some/path/test/path/to/feature` (mirrors `some/path/src/path/to/query`)
34+
- At least one test `c` or `c++` file, typically named `test.c`/`test.cpp`, with lines annotated `// COMPLIANT` or `// NON_COMPLIANT`
35+
- A `.ql` file with test query logic, or a `.qlref` file referring to the production query logic
36+
- A matching `FOO.expected` file to go with each `FOO.ql` or `FOO.qlref`, containing the test query results for the test `c` or `c++` files
37+
- Note that some test directories simply have a `testref` file, to document that a certain query is tested in a different directory.
38+
39+
As a code reviewer, it is critical to ensure that the results in the `.expected` file match the comments in the test file.
40+
41+
The `.expected` file uses a columnar format:
42+
- For example, a basic row may look like `| test.cpp:8:22:8:37 | element | message |`.
43+
- For a query with `select x, "test"`, the columns are | x.getLocation() | x.toString() | "test" |`
44+
- An alert with placeholders will use `$@` in the message, and have additional `element`/`string` columns for placeholder, e.g. `| test.cpp:8:22:8:37 | ... + ... | Invalid add of $@. | test.cpp:7:5:7:12 | my_var | deprecated variable my_var |`.
45+
- Remember, there is one `.expected` file for each `.ql` or `.qlref` file.
46+
- Each `.expected` file will contain the results for all test c/cpp files.
47+
- The `toString()` format of QL objects is deliberately terse for performance reasons.
48+
- For certain queries such as "path problems", the results may be grouped into categories via text lines with the category name, e.g. `nodes` and `edges` and `problems`.
49+
50+
Reviewing tests in this style can be tedious and error prone, but fundamental to the effectiveness of our TDD requirements in this project.
51+
52+
When reviewing tests, it is critical to:
53+
- Check that each `NON_COMPLIANT` case in the test file has a row in the correct `.expected` file referring to the correct location.
54+
- Check that each row in each `.expected` file has a `NON_COMPLIANT` case in the test file at the correct location.
55+
- Check that there are no `.expected` rows that refer to test code cases marked as `COMPLIANT`, or with no comment
56+
- Note that it is OK if the locations of the comment are not precisely aligned with the alert
57+
- Check that the alert message and placeholders are accurate and understandable.
58+
- Consider the "test coverage" of the query, are each of its logical statements effectively exercised individually, collectively? The test should neither be overly bloated nor under specified.
59+
- Consider the edge cases of the language itself, will the analysis work in non-trivial cases, are all relevant language concepts tested here? This doesn't need to be exhaustive, but it should be thoughfully thorough.

0 commit comments

Comments
 (0)