Shared: Inline test expectations #11778

yoff · 2022-12-21T21:15:49Z

Moves the inline test implementation to a shared module.

cpp/ql/test/TestUtilities/InlineExpectationsTest.qll

shared/util/codeql/util/test/InlineExpectationsTest.qll

- remove from identical-files

- add util shared pack to ql - remove from identical-files

- remove from identical-files

- add util shared pack to swift - remove from identical-files

erik-krogh · 2022-12-22T10:11:40Z

I had an idea of how to simplify the signature even further: We could make a newtype based implementation of Location inside the Make module, and by doing that just use a hasLocationInfo predicate instead of getLocation().

That way the signature just becomes a single class signature ExpectationCommentSig { .. }, which should be easier for new languages to implement.

I've implemented it as a PR against this PR here: yoff#37

jketema

When expanding some of the individual files in the "Files changed" view I still see some QL-for-QL warnings, e.g., on line 16 of the new cpp/ql/test/TestUtilities/InlineExpectationsTest.qll. Those should probably be resolved.

cpp/ql/test/TestUtilities/InlineExpectationsTest.qll

atorralba

Java mostly LGTM, with a small suggestion:

atorralba · 2022-12-22T14:26:25Z

java/ql/test/TestUtilities/InlineExpectationsTest.qll

-  GoodExpectation() { getKnownFailure() = "" }
-}
+  private class XmlExpectationComment extends ExpectationComment instanceof J::XmlComment {
+    override string getContents() { result = this.(J::XmlComment).getText().trim() }


Suggested change

override string getContents() { result = this.(J::XmlComment).getText().trim() }

override string getContents() { result = super.getText().trim() }

atorralba · 2022-12-22T14:26:25Z

java/ql/test/TestUtilities/InlineExpectationsTest.qll

-class FalsePositiveExpectation extends ValidExpectation {
-  FalsePositiveExpectation() { getKnownFailure() = "SPURIOUS" }
-}
+    override Location getLocation() { result = this.(J::XmlComment).getLocation() }


Suggested change

override Location getLocation() { result = this.(J::XmlComment).getLocation() }

override Location getLocation() { result = J::XmlComment.super.getLocation() }

atorralba · 2022-12-22T14:26:25Z

java/ql/test/TestUtilities/InlineExpectationsTest.qll

-  FalseNegativeExpectation() { getKnownFailure() = "MISSING" }
-}
+    override predicate hasLocationInfo(string path, int sl, int sc, int el, int ec) {
+      this.(J::XmlComment).hasLocationInfo(path, sl, sc, el, ec)


Suggested change

this.(J::XmlComment).hasLocationInfo(path, sl, sc, el, ec)

J::XmlComment.super.hasLocationInfo(path, sl, sc, el, ec)

atorralba · 2022-12-22T14:26:25Z

java/ql/test/TestUtilities/InlineExpectationsTest.qll

-  string expectation;
-
-  InvalidExpectation() { this = TInvalidExpectation(comment, expectation) }
+    override string toString() { result = this.(J::XmlComment).toString() }


Suggested change

override string toString() { result = this.(J::XmlComment).toString() }

override string toString() { result = J::XmlComment.super.toString() }

aibaars

👍 For Ruby

yoff · 2022-12-23T12:17:15Z

@erik-krogh nice, I agree we might prefer that..

inline Location into the shared implementation of InlineExpectationsTest

geoffw0

👍 for CPP and Swift, if the warnings pointed out by @jketema are addressed.

geoffw0 · 2023-01-04T16:54:02Z

config/identical-files.json

-    "ql/ql/test/TestUtilities/InlineExpectationsTest.qll",
-    "go/ql/test/TestUtilities/InlineExpectationsTest.qll",
-    "swift/ql/test/TestUtilities/InlineExpectationsTest.qll"
-  ],


Friday win for sure.

This reverts commit 65fe9ab, reversing changes made to 08e9d33.

yoff · 2023-01-05T08:21:53Z

Unfortunately we do have to assume a Location class, otherwise users cannot implement hasActualResult. (And I think that changing the signature of that predicate to not use Location would be cumbersome.)

erik-krogh

👍

go/ql/test/TestUtilities/InlineExpectationsTest.qll

Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>

atorralba

Java 👍

michaelnebel

C# LGTM!
Very nice application of param modules!!!

tausbn

Python 👍

github-actions bot added C# C++ Go Java Python QL-for-QL Ruby Swift labels Dec 21, 2022

github-code-scanning bot found potential problems Dec 21, 2022

View changes

yoff added 9 commits Dec 22, 2022

shared: Add shared inline expectation test library

b767dcf

Python: use shared inline tests

e8d3802

- remove from identical-files

cpp: use shared inline tests

d97e185

- remove from identical-files

csharp: use shared inline tests

a9b232b

- remove from identical-files

java: use shared inline tests

4667068

- remove from identical-files

ruby: use shared inline tests

0d6c643

- remove from identical-files

ql4ql: use shared inline tests

f28eb6b

- add util shared pack to ql - remove from identical-files

go: use shared inline tests

b0d7998

- remove from identical-files

swift: use shared inline tests

08e9d33

- add util shared pack to swift - remove from identical-files

yoff force-pushed the shared/inline-tests branch from d5ec32d to 08e9d33 Compare Dec 22, 2022

yoff marked this pull request as ready for review Dec 22, 2022

yoff requested review from a team as code owners Dec 22, 2022

inline Location into the shared implementation of InlineExpectationsTest

b3dd50b

jketema reviewed Dec 22, 2022

View changes

cpp/ql/test/TestUtilities/InlineExpectationsTest.qll Show resolved Hide resolved

atorralba reviewed Dec 22, 2022

View changes

aibaars previously approved these changes Dec 23, 2022

View changes

Merge pull request #37 from erik-krogh/shared/inline-tests

65fe9ab

inline Location into the shared implementation of InlineExpectationsTest

yoff dismissed aibaars’s stale review via 65fe9ab Jan 4, 2023

geoffw0 reviewed Jan 4, 2023

View changes

yoff added 2 commits Jan 4, 2023

java: nicer code as suggested by review

2e46919

Revert "Merge pull request #37 from erik-krogh/shared/inline-tests"

c3b3c05

This reverts commit 65fe9ab, reversing changes made to 08e9d33.

erik-krogh previously approved these changes Jan 5, 2023

View changes

owen-mc previously approved these changes Jan 5, 2023

View changes

go/ql/test/TestUtilities/InlineExpectationsTest.qll Outdated Show resolved Hide resolved

Update go/ql/test/TestUtilities/InlineExpectationsTest.qll

a74062c

Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com>

yoff dismissed stale reviews from owen-mc and erik-krogh via a74062c Jan 5, 2023

cpp/swift: fix qldoc

8afb541

yoff requested review from atorralba and jketema Jan 5, 2023

jketema previously approved these changes Jan 5, 2023

View changes

swift: fix typo

8d9e94a

yoff dismissed jketema’s stale review via 8d9e94a Jan 6, 2023

jketema approved these changes Jan 6, 2023

View changes

owen-mc approved these changes Jan 6, 2023

View changes

atorralba approved these changes Jan 9, 2023

View changes

erik-krogh approved these changes Jan 9, 2023

View changes

michaelnebel approved these changes Jan 9, 2023

View changes

calumgrant requested a review from tausbn Jan 9, 2023

tausbn approved these changes Jan 9, 2023

View changes

yoff merged commit c01ce95 into github:main Jan 9, 2023
44 checks passed

Shared: Inline test expectations #11778

Shared: Inline test expectations #11778

yoff commented Dec 21, 2022

erik-krogh commented Dec 22, 2022 •

edited

jketema left a comment

atorralba left a comment

atorralba Dec 22, 2022

atorralba Dec 22, 2022

atorralba Dec 22, 2022

atorralba Dec 22, 2022 •

edited

aibaars left a comment

yoff commented Dec 23, 2022

geoffw0 left a comment

geoffw0 Jan 4, 2023

erik-krogh Jan 4, 2023

yoff commented Jan 5, 2023

erik-krogh left a comment

atorralba left a comment

michaelnebel left a comment

tausbn left a comment

	override string getContents() { result = this.(J::XmlComment).getText().trim() }
	override string getContents() { result = super.getText().trim() }

	override Location getLocation() { result = this.(J::XmlComment).getLocation() }
	override Location getLocation() { result = J::XmlComment.super.getLocation() }

	this.(J::XmlComment).hasLocationInfo(path, sl, sc, el, ec)
	J::XmlComment.super.hasLocationInfo(path, sl, sc, el, ec)

	override string toString() { result = this.(J::XmlComment).toString() }
	override string toString() { result = J::XmlComment.super.toString() }

Shared: Inline test expectations #11778

Shared: Inline test expectations #11778

Conversation

yoff commented Dec 21, 2022

erik-krogh commented Dec 22, 2022 • edited

jketema left a comment

Choose a reason for hiding this comment

atorralba left a comment

Choose a reason for hiding this comment

atorralba Dec 22, 2022

Choose a reason for hiding this comment

atorralba Dec 22, 2022

Choose a reason for hiding this comment

atorralba Dec 22, 2022

Choose a reason for hiding this comment

atorralba Dec 22, 2022 • edited

Choose a reason for hiding this comment

aibaars left a comment

Choose a reason for hiding this comment

yoff commented Dec 23, 2022

geoffw0 left a comment

Choose a reason for hiding this comment

geoffw0 Jan 4, 2023

Choose a reason for hiding this comment

erik-krogh Jan 4, 2023

Choose a reason for hiding this comment

yoff commented Jan 5, 2023

erik-krogh left a comment

Choose a reason for hiding this comment

atorralba left a comment

Choose a reason for hiding this comment

michaelnebel left a comment

Choose a reason for hiding this comment

tausbn left a comment

Choose a reason for hiding this comment

erik-krogh commented Dec 22, 2022 •

edited

atorralba Dec 22, 2022 •

edited