|
| 1 | +--- |
| 2 | +description: 'Code review guidelines for GitHub copilot in this project' |
| 3 | +applyTo: '**' |
| 4 | +excludeAgent: ["coding-agent"] |
| 5 | +--- |
| 6 | + |
| 7 | +# Code Review Instructions |
| 8 | + |
| 9 | +A change note is required for any pull request which modifies: |
| 10 | +- The structure or layout of the release artifacts. |
| 11 | +- The evaluation performance (memory, execution time) of an existing query. |
| 12 | +- The results of an existing query in any circumstance. |
| 13 | + |
| 14 | +If the pull request only adds new rule queries, a change note is not required. |
| 15 | +Confirm that either a change note is not required or the change note is required and has been added. |
| 16 | + |
| 17 | +For PRs that add new queries or modify existing queries, also consider the following review checklist: |
| 18 | +- Confirm that the output format of shared queries is valid. |
| 19 | +- Have all the relevant rule package description files been checked in? |
| 20 | +- Have you verified that the metadata properties of each new query is set appropriately? |
| 21 | +- Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases? |
| 22 | +- Are all the alerts in the expected file annotated as NON_COMPLIANT in the test source file? |
| 23 | +- Are the alert messages properly formatted and consistent with the style guide? |
| 24 | +- Does the query have an appropriate level of in-query comments/documentation? |
| 25 | +- Does the query not reinvent features in the standard library? |
| 26 | +- Can the query be simplified further (not golfed!). |
| 27 | + |
| 28 | +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 | +- Check that the locations do not refer to files in the standard library, as these have issues in GitHub's Code Scanning UI and complicate our compiler compatibility tests. |
| 59 | +- 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. |
| 60 | +- 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