Dataflow: Add support for pretty-printed alert provenance in tests #16210
Conversation
aschackmull
added
the
no-change-note-required
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * @kind path-problem | ||
| */ | ||
|
|
||
| import codeql.ruby.AST |
Check warning
Code scanning / CodeQL
Redundant import Warning test
| | | ||
| sourceModel(namespace, type, subtypes, name, signature, ext, output, kind, provenance, madId) and | ||
| model = | ||
| "Source: " + namespace + "; " + type + "; " + subtypes + "; " + name + "; " + signature + "; " |
There was a problem hiding this comment.
Maybe just use ; as separator (this is what we typically do in other places we print models).
There was a problem hiding this comment.
This string is only for inclusion in test output, and I found that it was more readable with the space included. Originally, before we had MaD rows in external yml files, I went with the no-space separation for the QL embedded csv rows in the name of compactness, but that's not relevant here.
| | | ||
| sinkModel(namespace, type, subtypes, name, signature, ext, input, kind, provenance, madId) and | ||
| model = | ||
| "Sink: " + namespace + "; " + type + "; " + subtypes + "; " + name + "; " + signature + "; " + |
| summaryModel(namespace, type, subtypes, name, signature, ext, input, output, kind, provenance, | ||
| madId) and | ||
| model = | ||
| "Summary: " + namespace + "; " + type + "; " + subtypes + "; " + name + "; " + signature + |
| ) | ||
| } | ||
|
|
||
| query predicate edges(PathNode a, PathNode b, string key, string val) { |
There was a problem hiding this comment.
Is there a reason for not printing the model itself instead of its "translated" id? Even though it is less likely, we would still need to update tests in case models suddenly swap order (not sure how stable the QlBuiltsins::ExtensionId is)? Furthermore, it might be easier to "identify" the model used by the edge from MaD format instead of an integer.
There was a problem hiding this comment.
Also, the translated ids could change in case a new edge is added that uses a model, which changes (pushes) the ranking (compared to the last time the test was run). This could mean that a large "unrelated" part of an expected file needs to be updated (which might cause confusion)
There was a problem hiding this comment.
I agree that this could be a source of irrelevant test changes, which we want to do our best to avoid. The current PR is an improvement, but Michael's suggestion of printing the whole model, while verbose, sounds like it would totally avoid the problem.
There was a problem hiding this comment.
I don't believe order swapping to be very likely, although @dbartol knows the full details for a more precise answer to that.
I do realise that there's shifting going on when a test changes the set of models that it's referring to, but I thought it was nicer to have the models on the side in order to have a narrower edges relation (narrow in the sense that the row in the .expected file fits within the width of a reasonably sized editor window). And balancing these two things, I think I'd prefer narrower edges over no shifting.
There was a problem hiding this comment.
That is a valid argument. I am just worried that "interesting" updates to the expected file could drown in noise. Also, it introduces added complexity to the expected files that one needs to know of the possibility of shifting.
It is ok with me, if we stick with the current solution, if we are willing to change it in case we frequently run into the problem above.
|
Just to be clear, applying this so that we don't get spurious changes in edge provenance when a new model is added will pretty much mean getting rid of |
aschackmull
force-pushed
the
dataflow/provenance-for-tests
branch
from
7fddee9
to
d6fc62a
Compare
|
I've fixed the merge conflicts, so if you want we can merge this. |
aschackmull
force-pushed
the
dataflow/provenance-for-tests
branch
from
d6fc62a
to
eda5073
Compare
aschackmull
force-pushed
the
dataflow/provenance-for-tests
branch
from
eda5073
to
7e980d9
Compare
|
Rebased again. |
This adds the list of referenced models in a qltest and renumbers the MaD ids to get stable test output. Where applicable, I've updated one test per language to demonstrate the conversion.