C++: Exclude tests in model generation #19498
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates model generation to skip functions located in test directories while still including the CodeQL summary tests under test/library-tests/dataflow/modelgenerator/dataflow.
- Adds a new predicate clause in
isUninterestingForModelsto exclude files whose paths containtest - Introduces an exception to retain the CodeQL summary tests directory
- Adjusts path-matching logic in
CaptureModels.qll
Comments suppressed due to low confidence (1)
cpp/ql/src/utils/modelgenerator/internal/CaptureModels.qll:49
- Add unit tests or QL tests to validate this exclusion rule, ensuring functions in general
testfolders are skipped and those undertest/library-tests/dataflow/modelgenerator/dataflowremain included.
// Exclude functions in test directories (but not the ones in the CodeQL test directory)
| // Exclude functions in test directories (but not the ones in the CodeQL test directory) | ||
| exists(Cpp::File f | | ||
| f = api.getFile() and | ||
| f.getAbsolutePath().matches("%test%") and |
There was a problem hiding this comment.
The pattern "%test%" may match any substring (e.g., "contest" or user paths containing "test"). Consider anchoring to directory separators (e.g., "%/test/%") or using a path-matching utility to target only "test" directories.
| f.getAbsolutePath().matches("%test%") and | |
| f.getAbsolutePath().matches("%/test/%") and |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is not a bad suggestion! I do like simply using test though so that we also match stuff like tests, though. I could go either way on this 🤷
| f.getAbsolutePath().matches("%test%") and | ||
| not f.getAbsolutePath().matches("%test/library-tests/dataflow/modelgenerator/dataflow/%") |
There was a problem hiding this comment.
Relying on absolute paths with OS-specific separators can reduce portability. Consider using getRelativePath() or normalizing paths with forward slashes to ensure this exception works across platforms.
| f.getAbsolutePath().matches("%test%") and | |
| not f.getAbsolutePath().matches("%test/library-tests/dataflow/modelgenerator/dataflow/%") | |
| f.getRelativePath().matches("%test%") and | |
| not f.getRelativePath().matches("%test/library-tests/dataflow/modelgenerator/dataflow/%") |
Copilot uses AI. Check for mistakes.
@jketema noticed that there are a few summaries (for OpenSSL specifically) in #19492 that are from test functions. This PR excludes model generation for functions inside folders that match
%test%.Care is needed to not exclude our own tests, though, as the summary tests are located in
cpp/ql/test/library-tests/dataflow/modelgenerator/dataflow. So we explicitly allow that directory.I tried to regenerate the OpenSSL models with these changes on top of #19492 and compare the changes: https://www.diffchecker.com/z6iyKLAU/ (this isn't super readable, but I've manually checked most of the excludes functions and they do indeed belong to tests)