Python: MaD on externals #13935
Python: MaD on externals #13935
Conversation
For this test, we can simply use the convention, that a file called `[ql-file-stem].ext.yml` will be used as data extensions exactly for the test represented by `ql-file`.
For these tests, we cannot use the same mechanism, as we want the data extensions to be available for both tests. Instead, we create a ql-pack for the test directory and point to the data entensions from there. This makes the extensions available for all tests in the directory.
98c9425
to
dbc6014
Compare
There was a problem hiding this comment.
I don't think the approach of changing codeql-workspace.yml for the sake of testing MaD in extension files is good. I can think of two alternative ways to fix this:
- convince core team to allow a
<foo>.ext.ymlto apply to all tests within a folder, instead of just<foo>.ql - merge the content of
InlineTaintTest.qlandNormalDataflowTest.qlto one file (such asAllTests.ql)
I first did 2, but it was awkward because I had to import the test modules and then re-export all the query predicates in a manner avoiding name clashes (these clashes cannot be resolved in the imported modules since they come from them being instantiations of the same parameterised module). This, apart from looking clunky, incurred a new burden of maintenance. I notice that If we really do not like to add to the workspace, I think we should go for a third option until something like 1 is implemented:
|
Oh no, I had not anticipated that 😐 I'm not sure whether I like your new solution 3 or the original version in the PR better... I honestly don't fancy either of them 😅 @aeisenberg since you've been heavily involved with both data extensions and the workspace file, could we get your opinion on how to add data-extension files in tests that contribute to multiple .ql files? |
I'd have to check to see if this would break any existing tests. I know the java tests uses this feature heavily. It's not a bad idea otherwise. For now, I'd lean towards option 3, which is not so great either, but easier for now and we can possibly implement option 1 in the future. |
|
Would you mind raising an issue for us to look into option 1? Are you thinking that you will be using this pattern often? |
I am not sure, actually. This one is certainly special since it is a test of the feature rather than a test using the feature. I anticipate that more of the latter will appear, but they may all be covered by the current mechanism. |
|
Thanks for the explanation. My opinion is that unless and until we start seeing this use case more often, we should not make any changes to the CLI to support it. |
This PR rewrites our tests of MaD via flow summaries, such that they use data extensions instead of CVS-row-predicates. This serves two purposes:
We also switch to the agreed naming convention for data extensions. Since we do not use data extensions yet, the switch only involves the obligatory empty extension.