Rework more inline expectation tests to use the parameterized module #13498
Conversation
![github-advanced-security[bot]](https://ssr-www.pages.dev/web/20260106123754im_/https://avatars.githubusercontent.com/in/57789?s=60&v=4&kwid=1tfa-bq%2560kjuf-lqd){kind=small}
| import MakeTest<MergeTests5<MergeTests5<SystemCommandExecutionTest, DecodingTest, EncodingTest, LoggingTest, | ||
| CodeExecutionTest>, | ||
| MergeTests5<SqlConstructionTest, SqlExecutionTest, XPathConstructionTest, XPathExecutionTest, | ||
| EscapingTest>, | ||
| MergeTests4<HttpServerRouteSetupTest, HttpServerRequestHandlerTest, | ||
| HttpServerHttpRedirectResponseTest, HttpServerCookieWriteTest>, | ||
| MergeTests5<FileSystemAccessTest, FileSystemWriteAccessTest, PathNormalizationTest, | ||
| SafeAccessCheckTest, PublicKeyGenerationTest>, | ||
| MergeTests5<CryptographicOperationTest, HttpClientRequestTest, CsrfProtectionSettingTest, | ||
| CsrfLocalProtectionSettingTest, XmlParsingTest>>> |
There was a problem hiding this comment.
I'm not particularly proud of this, but I don't see a better way of doing this with 25 inline expectation tests in a single file.
Note that HttpServerHttpResponseTest is still missing here. That remains one of the inline expectation tests that still needs to be reworked.
| import experimental.meta.InlineTaintTest | ||
| import MakeInlineTaintTest<TestTaintTrackingConfig> |
There was a problem hiding this comment.
I wonder if this could be put into a file, say DefaultInlineTaintTest.qll that is then imported. Just since this is the way we always use it...
There was a problem hiding this comment.
I generally did that when the config and the Make.... were in the same file and when it was clear to me that it would not be sensible to use any other configuration. If you think that the latter applies here (i.e., no other sensible configuration), then I happily move this.
There was a problem hiding this comment.
It might be sensible we just do not really do it. That is why I suggested a convenience file so a single import would be enough in most cases. It could even be named InlineTaintTest to remove the common case form the diff of this PR; then we just have to come up with a different name for the unspecialised one...
There was a problem hiding this comment.
I do not think such an ergonomic improvement has to be part of this PR, though. The common workflow is to just copy an existing version of the file into a new test folder, so the number of lines in the file does not really matter inthat case...
There was a problem hiding this comment.
Then I'll leave that improvement to the Python team.
yoff
left a comment
yoff
left a comment
There was a problem hiding this comment.
Thanks for this enormous effort 💪
4 participants
Follow up to #12789, #13346, and #13426. This PR introduces three additional merge modules to merge 3, 4, and 5 inline expectation tests, as Go and Python have a few instances where that many inline expectation tests occur in a single file. This solution seemed cleaner than having nested merged test in the test ql files, although in one case that is still necessary (see comment below).
Other than the above, there's one commit per language. The Go and Ruby ones deal with the last remaining cases for those languages where the parameterized module was not used. In the case of Python there are two remaining instances that still use the inline expectations test class.