Merge pull request #13643 from jketema/inline-5

Rework the remaining inline expectation tests to use the parameterized module

python/ql/test/experimental/dataflow/TestUtil/FlowTest.qll

@@ -9,7 +9,7 @@ signature module FlowTestSig {
   predicate relevantFlow(DataFlow::Node fromNode, DataFlow::Node toNode);
 }
 
-private module FlowTest<FlowTestSig Impl> implements TestSig {
+module MakeTestSig<FlowTestSig Impl> implements TestSig {
   string getARelevantTag() { result = Impl::flowTag() }
 
   predicate hasActualResult(Location location, string element, string tag, string value) {
@@ -37,11 +37,3 @@ private module FlowTest<FlowTestSig Impl> implements TestSig {
     )
   }
 }
-
-module MakeFlowTest<FlowTestSig Impl> {
-  import MakeTest<FlowTest<Impl>>
-}
-
-module MakeFlowTest2<FlowTestSig Impl1, FlowTestSig Impl2> {
-  import MakeTest<MergeTests<FlowTest<Impl1>, FlowTest<Impl2>>>
-}

python/ql/test/experimental/dataflow/TestUtil/LocalFlowStepTest.qll

@@ -10,4 +10,4 @@ module LocalFlowStepTest implements FlowTestSig {
   }
 }
 
-import MakeFlowTest<LocalFlowStepTest>
+import MakeTest<MakeTestSig<LocalFlowStepTest>>

python/ql/test/experimental/dataflow/TestUtil/MaximalFlowTest.qll

@@ -12,7 +12,7 @@ module MaximalFlowTest implements FlowTestSig {
   }
 }
 
-import MakeFlowTest<MaximalFlowTest>
+import MakeTest<MakeTestSig<MaximalFlowTest>>
 
 /**
  * A configuration to find all "maximal" flows.

python/ql/test/experimental/dataflow/TestUtil/NormalDataflowTest.qll

@@ -11,7 +11,7 @@ module DataFlowTest implements FlowTestSig {
   }
 }
 
-import MakeFlowTest<DataFlowTest>
+import MakeTest<MakeTestSig<DataFlowTest>>
 
 query predicate missingAnnotationOnSink(Location location, string error, string element) {
   error = "ERROR, you should add `# $ MISSING: flow` annotation" and

python/ql/test/experimental/dataflow/TestUtil/NormalTaintTrackingTest.qll

@@ -11,7 +11,7 @@ module DataFlowTest implements FlowTestSig {
   }
 }
 
-import MakeFlowTest<DataFlowTest>
+import MakeTest<MakeTestSig<DataFlowTest>>
 
 query predicate missingAnnotationOnSink(Location location, string error, string element) {
   error = "ERROR, you should add `# $ MISSING: flow` annotation" and

python/ql/test/experimental/dataflow/TestUtil/RoutingTest.qll

@@ -10,22 +10,25 @@ private import semmle.python.dataflow.new.internal.DataFlowPrivate as DataFlowPr
  * the functions tested sink their arguments sequentially, that is
  * `SINK1(arg1)`, etc.
  */
-abstract class RoutingTest extends InlineExpectationsTest {
-  bindingset[this]
-  RoutingTest() { any() }
+signature module RoutingTestSig {
+  class Argument;
 
-  abstract string flowTag();
+  string flowTag(Argument arg);
 
-  abstract predicate relevantFlow(DataFlow::Node fromNode, DataFlow::Node toNode);
+  predicate relevantFlow(DataFlow::Node fromNode, DataFlow::Node toNode, Argument arg);
+}
 
-  override string getARelevantTag() { result in ["func", this.flowTag()] }
+module MakeTestSig<RoutingTestSig Impl> implements TestSig {
+  string getARelevantTag() { result in ["func", Impl::flowTag(_)] }
 
-  override predicate hasActualResult(Location location, string element, string tag, string value) {
-    exists(DataFlow::Node fromNode, DataFlow::Node toNode | this.relevantFlow(fromNode, toNode) |
+  predicate hasActualResult(Location location, string element, string tag, string value) {
+    exists(DataFlow::Node fromNode, DataFlow::Node toNode, Impl::Argument arg |
+      Impl::relevantFlow(fromNode, toNode, arg)
+    |
       location = fromNode.getLocation() and
       element = fromNode.toString() and
       (
-        tag = this.flowTag() and
+        tag = Impl::flowTag(arg) and
         if "\"" + tag + "\"" = fromValue(fromNode) then value = "" else value = fromValue(fromNode)
         or
         // only have result for `func` tag if the function where `arg<n>` is used, is

python/ql/test/experimental/dataflow/coverage-py2/argumentRoutingTest.expected

@@ -0,0 +1,2 @@
+failures
+testFailures

python/ql/test/experimental/dataflow/coverage-py3/argumentRoutingTest.expected

@@ -0,0 +1,2 @@
+failures
+testFailures

python/ql/test/experimental/dataflow/coverage/argumentRoutingTest.expected

@@ -0,0 +1,2 @@
+failures
+testFailures

python/ql/test/experimental/dataflow/coverage/argumentRoutingTest.ql

@@ -3,19 +3,22 @@ import semmle.python.dataflow.new.DataFlow
 private import semmle.python.dataflow.new.internal.DataFlowPrivate as DataFlowPrivate
 import experimental.dataflow.TestUtil.RoutingTest
 
-class Argument1RoutingTest extends RoutingTest {
-  Argument1RoutingTest() { this = "Argument1RoutingTest" }
+module Argument1RoutingTest implements RoutingTestSig {
+  class Argument = Unit;
 
-  override string flowTag() { result = "arg1" }
+  string flowTag(Argument arg) { result = "arg1" and exists(arg) }
 
-  override predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink) {
-    exists(Argument1ExtraRoutingConfig cfg | cfg.hasFlow(source, sink))
-    or
-    exists(ArgumentRoutingConfig cfg |
-      cfg.hasFlow(source, sink) and
-      cfg.isArgSource(source, 1) and
-      cfg.isGoodSink(sink, 1)
-    )
+  predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink, Argument arg) {
+    (
+      exists(Argument1ExtraRoutingConfig cfg | cfg.hasFlow(source, sink))
+      or
+      exists(ArgumentRoutingConfig cfg |
+        cfg.hasFlow(source, sink) and
+        cfg.isArgSource(source, 1) and
+        cfg.isGoodSink(sink, 1)
+      )
+    ) and
+    exists(arg)
   }
 }
 
@@ -87,59 +90,54 @@ class Argument1ExtraRoutingConfig extends DataFlow::Configuration {
   override predicate isBarrierIn(DataFlow::Node node) { this.isSource(node) }
 }
 
-class RestArgumentRoutingTest extends RoutingTest {
-  ArgNumber argNumber;
+module RestArgumentRoutingTest implements RoutingTestSig {
+  class Argument = ArgNumber;
 
-  RestArgumentRoutingTest() {
-    argNumber > 1 and
-    this = "Argument" + argNumber + "RoutingTest"
-  }
+  string flowTag(Argument arg) { result = "arg" + arg }
 
-  override string flowTag() { result = "arg" + argNumber }
-
-  override predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink) {
+  predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink, Argument arg) {
     exists(ArgumentRoutingConfig cfg |
       cfg.hasFlow(source, sink) and
-      cfg.isArgSource(source, argNumber) and
-      cfg.isGoodSink(sink, argNumber)
-    )
+      cfg.isArgSource(source, arg) and
+      cfg.isGoodSink(sink, arg)
+    ) and
+    arg > 1
   }
 }
 
 /** Bad flow from `arg<n>` to `SINK<N>_F` */
-class BadArgumentRoutingTestSinkF extends RoutingTest {
-  ArgNumber argNumber;
+module BadArgumentRoutingTestSinkF implements RoutingTestSig {
+  class Argument = ArgNumber;
 
-  BadArgumentRoutingTestSinkF() { this = "BadArgumentRoutingTestSinkF" + argNumber }
+  string flowTag(Argument arg) { result = "bad" + arg }
 
-  override string flowTag() { result = "bad" + argNumber }
-
-  override predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink) {
+  predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink, Argument arg) {
     exists(ArgumentRoutingConfig cfg |
       cfg.hasFlow(source, sink) and
-      cfg.isArgSource(source, argNumber) and
-      cfg.isBadSink(sink, argNumber)
+      cfg.isArgSource(source, arg) and
+      cfg.isBadSink(sink, arg)
     )
   }
 }
 
 /** Bad flow from `arg<n>` to `SINK<M>` or `SINK<M>_F`, where `n != m`. */
-class BadArgumentRoutingTestWrongSink extends RoutingTest {
-  ArgNumber argNumber;
+module BadArgumentRoutingTestWrongSink implements RoutingTestSig {
+  class Argument = ArgNumber;
 
-  BadArgumentRoutingTestWrongSink() { this = "BadArgumentRoutingTestWrongSink" + argNumber }
+  string flowTag(Argument arg) { result = "bad" + arg }
 
-  override string flowTag() { result = "bad" + argNumber }
-
-  override predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink) {
+  predicate relevantFlow(DataFlow::Node source, DataFlow::Node sink, Argument arg) {
     exists(ArgumentRoutingConfig cfg |
       cfg.hasFlow(source, sink) and
-      cfg.isArgSource(source, any(ArgNumber i | not i = argNumber)) and
+      cfg.isArgSource(source, any(ArgNumber i | not i = arg)) and
       (
-        cfg.isGoodSink(sink, argNumber)
+        cfg.isGoodSink(sink, arg)
         or
-        cfg.isBadSink(sink, argNumber)
+        cfg.isBadSink(sink, arg)
       )
     )
   }
 }
+
+import MakeTest<MergeTests4<MakeTestSig<Argument1RoutingTest>, MakeTestSig<RestArgumentRoutingTest>,
+  MakeTestSig<BadArgumentRoutingTestSinkF>, MakeTestSig<BadArgumentRoutingTestWrongSink>>>

python/ql/test/experimental/dataflow/module-initialization/localFlow.ql

@@ -31,4 +31,4 @@ module RuntimeLocalFlowTest implements FlowTestSig {
   }
 }
 
-import MakeFlowTest2<ImportTimeLocalFlowTest, RuntimeLocalFlowTest>
+import MakeTest<MergeTests<MakeTestSig<ImportTimeLocalFlowTest>, MakeTestSig<RuntimeLocalFlowTest>>>

python/ql/test/experimental/dataflow/typetracking-summaries/tracked.expected

@@ -0,0 +1,2 @@
+failures
+testFailures

python/ql/test/experimental/dataflow/typetracking-summaries/tracked.ql

@@ -15,12 +15,10 @@ private DataFlow::TypeTrackingNode tracked(TypeTracker t) {
   exists(TypeTracker t2 | result = tracked(t2).track(t2, t))
 }
 
-class TrackedTest extends InlineExpectationsTest {
-  TrackedTest() { this = "TrackedTest" }
+module TrackedTest implements TestSig {
+  string getARelevantTag() { result = "tracked" }
 
-  override string getARelevantTag() { result = "tracked" }
-
-  override predicate hasActualResult(Location location, string element, string tag, string value) {
+  predicate hasActualResult(Location location, string element, string tag, string value) {
     exists(DataFlow::Node e, TypeTracker t |
       exists(e.getLocation().getFile().getRelativePath()) and
       e.getLocation().getStartLine() > 0 and
@@ -34,3 +32,5 @@ class TrackedTest extends InlineExpectationsTest {
     )
   }
 }
+
+import MakeTest<TrackedTest>

python/ql/test/experimental/library-tests/CallGraph/InlineCallGraphTest.ql

@@ -108,7 +108,7 @@ query predicate pointsTo_found_typeTracker_notFound(CallNode call, string qualna
     not typeTrackerCallEdge(call, target) and
     qualname = getCallEdgeValue(call, target) and
     // ignore SPURIOUS call edges
-    not exists(FalsePositiveExpectation spuriousResult |
+    not exists(FalsePositiveTestExpectation spuriousResult |
       spuriousResult.getTag() = "pt" and
       spuriousResult.getValue() = getCallEdgeValue(call, target) and
       spuriousResult.getLocation().getFile() = call.getLocation().getFile() and
@@ -127,7 +127,7 @@ query predicate typeTracker_found_pointsTo_notFound(CallNode call, string qualna
     // between the two).
     not typeTrackerClassCall(call, target) and
     // ignore SPURIOUS call edges
-    not exists(FalsePositiveExpectation spuriousResult |
+    not exists(FalsePositiveTestExpectation spuriousResult |
       spuriousResult.getTag() = "tt" and
       spuriousResult.getValue() = getCallEdgeValue(call, target) and
       spuriousResult.getLocation().getFile() = call.getLocation().getFile() and

python/ql/test/experimental/meta/ConceptsTest.qll

@@ -255,52 +255,64 @@ module HttpServerRequestHandlerTest implements TestSig {
   }
 }
 
-class HttpServerHttpResponseTest extends InlineExpectationsTest {
-  File file;
+abstract class DedicatedResponseTest extends string {
+  bindingset[this]
+  DedicatedResponseTest() { any() }
 
-  HttpServerHttpResponseTest() {
-    file.getExtension() = "py" and
-    this = "HttpServerHttpResponseTest: " + file
-  }
+  string toString() { result = this }
 
-  override string getARelevantTag() { result in ["HttpResponse", "responseBody", "mimetype"] }
+  abstract predicate isDedicatedFile(File file);
+}
 
-  override predicate hasActualResult(Location location, string element, string tag, string value) {
+module HttpServerHttpResponseTest implements TestSig {
+  string getARelevantTag() { result in ["HttpResponse", "responseBody", "mimetype"] }
+
+  predicate hasActualResult(Location location, string element, string tag, string value) {
     // By adding `file` as a class field, and these two restrictions, it's possible to
     // say that we only want to check _some_ tags for certain files. This helped make
     // flask tests more readable since adding full annotations for HttpResponses in the
     // the tests for routing setup is both annoying and not very useful.
-    location.getFile() = file and
-    exists(file.getRelativePath()) and
-    // we need to do this step since we expect subclasses could override getARelevantTag
-    tag = this.getARelevantTag() and
-    (
-      exists(Http::Server::HttpResponse response |
-        location = response.getLocation() and
-        element = response.toString() and
-        value = "" and
-        tag = "HttpResponse"
-      )
-      or
-      exists(Http::Server::HttpResponse response |
-        location = response.getLocation() and
-        element = response.toString() and
-        value = prettyNodeForInlineTest(response.getBody()) and
-        tag = "responseBody"
-      )
-      or
-      exists(Http::Server::HttpResponse response |
-        location = response.getLocation() and
-        element = response.toString() and
-        // Ensure that an expectation value such as "mimetype=text/html; charset=utf-8" is parsed as a
-        // single expectation with tag mimetype, and not as two expectations with tags mimetype and
-        // charset.
+    exists(File file |
+      location.getFile() = file and
+      file.getExtension() = "py" and
+      exists(file.getRelativePath()) and
+      // we need to do this step since we expect subclasses could override getARelevantTag
+      tag = getARelevantTag() and
+      (
+        exists(Http::Server::HttpResponse response |
+          location = response.getLocation() and
+          element = response.toString() and
+          value = "" and
+          tag = "HttpResponse"
+        )
+        or
         (
-          if exists(response.getMimetype().indexOf(" "))
-          then value = "\"" + response.getMimetype() + "\""
-          else value = response.getMimetype()
+          not exists(DedicatedResponseTest d)
+          or
+          exists(DedicatedResponseTest d | d.isDedicatedFile(file))
         ) and
-        tag = "mimetype"
+        (
+          exists(Http::Server::HttpResponse response |
+            location = response.getLocation() and
+            element = response.toString() and
+            value = prettyNodeForInlineTest(response.getBody()) and
+            tag = "responseBody"
+          )
+          or
+          exists(Http::Server::HttpResponse response |
+            location = response.getLocation() and
+            element = response.toString() and
+            // Ensure that an expectation value such as "mimetype=text/html; charset=utf-8" is parsed as a
+            // single expectation with tag mimetype, and not as two expectations with tags mimetype and
+            // charset.
+            (
+              if exists(response.getMimetype().indexOf(" "))
+              then value = "\"" + response.getMimetype() + "\""
+              else value = response.getMimetype()
+            ) and
+            tag = "mimetype"
+          )
+        )
       )
     )
   }
@@ -545,7 +557,7 @@ import MakeTest<MergeTests5<MergeTests5<SystemCommandExecutionTest, DecodingTest
     CodeExecutionTest>,
   MergeTests5<SqlConstructionTest, SqlExecutionTest, XPathConstructionTest, XPathExecutionTest,
     EscapingTest>,
-  MergeTests4<HttpServerRouteSetupTest, HttpServerRequestHandlerTest,
+  MergeTests5<HttpServerRouteSetupTest, HttpServerRequestHandlerTest, HttpServerHttpResponseTest,
     HttpServerHttpRedirectResponseTest, HttpServerCookieWriteTest>,
   MergeTests5<FileSystemAccessTest, FileSystemWriteAccessTest, PathNormalizationTest,
     SafeAccessCheckTest, PublicKeyGenerationTest>,

python/ql/test/library-tests/frameworks/aiohttp/ConceptsTest.ql

@@ -1,12 +1,8 @@
 import python
 import experimental.meta.ConceptsTest
 
-class DedicatedResponseTest extends HttpServerHttpResponseTest {
-  DedicatedResponseTest() { file.getShortName() = "response_test.py" }
-}
+class DedicatedTest extends DedicatedResponseTest {
+  DedicatedTest() { this = "response_test.py" }
 
-class OtherResponseTest extends HttpServerHttpResponseTest {
-  OtherResponseTest() { not this instanceof DedicatedResponseTest }
-
-  override string getARelevantTag() { result = "HttpResponse" }
+  override predicate isDedicatedFile(File file) { file.getShortName() = this }
 }

python/ql/test/library-tests/frameworks/django-v1/ConceptsTest.ql

@@ -1,12 +1,8 @@
 import python
 import experimental.meta.ConceptsTest
 
-class DedicatedResponseTest extends HttpServerHttpResponseTest {
-  DedicatedResponseTest() { file.getShortName() = "response_test.py" }
-}
+class DedicatedTest extends DedicatedResponseTest {
+  DedicatedTest() { this = "response_test.py" }
 
-class OtherResponseTest extends HttpServerHttpResponseTest {
-  OtherResponseTest() { not this instanceof DedicatedResponseTest }
-
-  override string getARelevantTag() { result = "HttpResponse" }
+  override predicate isDedicatedFile(File file) { file.getShortName() = this }
 }

python/ql/test/library-tests/frameworks/django-v2-v3/ConceptsTest.ql

@@ -1,12 +1,8 @@
 import python
 import experimental.meta.ConceptsTest
 
-class DedicatedResponseTest extends HttpServerHttpResponseTest {
-  DedicatedResponseTest() { file.getShortName() = "response_test.py" }
-}
+class DedicatedTest extends DedicatedResponseTest {
+  DedicatedTest() { this = "response_test.py" }
 
-class OtherResponseTest extends HttpServerHttpResponseTest {
-  OtherResponseTest() { not this instanceof DedicatedResponseTest }
-
-  override string getARelevantTag() { result = "HttpResponse" }
+  override predicate isDedicatedFile(File file) { file.getShortName() = this }
 }

python/ql/test/library-tests/frameworks/fastapi/ConceptsTest.ql

@@ -1,12 +1,8 @@
 import python
 import experimental.meta.ConceptsTest
 
-class DedicatedResponseTest extends HttpServerHttpResponseTest {
-  DedicatedResponseTest() { file.getShortName() = "response_test.py" }
-}
+class DedicatedTest extends DedicatedResponseTest {
+  DedicatedTest() { this = "response_test.py" }
 
-class OtherResponseTest extends HttpServerHttpResponseTest {
-  OtherResponseTest() { not this instanceof DedicatedResponseTest }
-
-  override string getARelevantTag() { result = "HttpResponse" }
+  override predicate isDedicatedFile(File file) { file.getShortName() = this }
 }

python/ql/test/library-tests/frameworks/flask/ConceptsTest.ql

@@ -1,12 +1,8 @@
 import python
 import experimental.meta.ConceptsTest
 
-class DedicatedResponseTest extends HttpServerHttpResponseTest {
-  DedicatedResponseTest() { file.getShortName() = "response_test.py" }
-}
+class DedicatedTest extends DedicatedResponseTest {
+  DedicatedTest() { this = "response_test.py" }
 
-class OtherResponseTest extends HttpServerHttpResponseTest {
-  OtherResponseTest() { not this instanceof DedicatedResponseTest }
-
-  override string getARelevantTag() { result = "HttpResponse" }
+  override predicate isDedicatedFile(File file) { file.getShortName() = this }
 }

python/ql/test/library-tests/frameworks/flask_admin/ConceptsTest.ql

@@ -1,12 +1,8 @@
 import python
 import experimental.meta.ConceptsTest
 
-class DedicatedResponseTest extends HttpServerHttpResponseTest {
-  DedicatedResponseTest() { file.getShortName() = "response_test.py" }
-}
+class DedicatedTest extends DedicatedResponseTest {
+  DedicatedTest() { this = "response_test.py" }
 
-class OtherResponseTest extends HttpServerHttpResponseTest {
-  OtherResponseTest() { not this instanceof DedicatedResponseTest }
-
-  override string getARelevantTag() { result = "HttpResponse" }
+  override predicate isDedicatedFile(File file) { file.getShortName() = this }
 }

python/ql/test/library-tests/frameworks/tornado/ConceptsTest.ql

@@ -1,12 +1,8 @@
 import python
 import experimental.meta.ConceptsTest
 
-class DedicatedResponseTest extends HttpServerHttpResponseTest {
-  DedicatedResponseTest() { file.getShortName() = "response_test.py" }
-}
+class DedicatedTest extends DedicatedResponseTest {
+  DedicatedTest() { this = "response_test.py" }
 
-class OtherResponseTest extends HttpServerHttpResponseTest {
-  OtherResponseTest() { not this instanceof DedicatedResponseTest }
-
-  override string getARelevantTag() { result = "HttpResponse" }
+  override predicate isDedicatedFile(File file) { file.getShortName() = this }
 }

python/ql/test/library-tests/frameworks/twisted/ConceptsTest.ql

@@ -1,12 +1,8 @@
 import python
 import experimental.meta.ConceptsTest
 
-class DedicatedResponseTest extends HttpServerHttpResponseTest {
-  DedicatedResponseTest() { file.getShortName() = "response_test.py" }
-}
+class DedicatedTest extends DedicatedResponseTest {
+  DedicatedTest() { this = "response_test.py" }
 
-class OtherResponseTest extends HttpServerHttpResponseTest {
-  OtherResponseTest() { not this instanceof DedicatedResponseTest }
-
-  override string getARelevantTag() { result = "HttpResponse" }
+  override predicate isDedicatedFile(File file) { file.getShortName() = this }
 }