Skip to content

Convert remaining {go,swift,ruby}-code-scanning.qls query tests to .qlref - #19817

Merged
d10c merged 9 commits into
github:mainfrom
d10c:d10c/convert-tests-to-qlref
Jun 24, 2025
Merged

Convert remaining {go,swift,ruby}-code-scanning.qls query tests to .qlref#19817
d10c merged 9 commits into
github:mainfrom
d10c:d10c/convert-tests-to-qlref

Conversation

@d10c

@d10cd10c commented Jun 19, 2025

Copy link
Copy Markdown
Contributor

Converts the remaining {go,swift,ruby}-code-scanning.qls query tests to .qlref.

Example prior work: #18848

In the Go IncorrectIntegerConversion case, the #select, edges, and nodes query predicates have different results depending on whether the Go source is compiled under 64-bit or 32-bit mode. So I have filtered out those predicates using a custom test post-processor.

Also, in the Ruby case I've made the utils/test/PrettyPrintModels.ql test postprocessor available; Swift apparently doesn't support the Models-as-Data extensible predicates, so I couldn't fit its existing MaD support into the codeql.dataflow.test.ProvenancePathGraph::TestPostProcessing::TranslateProvenanceResults<> interface.

Anecdotally, the Copilot "Next Edit Suggestion" feature had some marginal benefit for this PR.

@d10cd10c changed the title Convert $lang-code-scanning.qls query tests to .qlrefConvert remaining {go,swift,ruby,java}-code-scanning.qls query tests to .qlrefJun 19, 2025
@d10cd10c added the no-change-note-required This PR does not need a change note label Jun 20, 2025
@d10c
d10c requested a review from a teamJune 20, 2025 08:32
@d10cd10c changed the title Convert remaining {go,swift,ruby,java}-code-scanning.qls query tests to .qlrefConvert remaining {go,swift,ruby}-code-scanning.qls query tests to .qlrefJun 20, 2025
@d10c
d10cforce-pushed the d10c/convert-tests-to-qlref branch 5 times, most recently from 4a12777 to 2aaa656CompareJune 20, 2025 13:47
Comment threadgo/ql/test/query-tests/Security/CWE-681/FilterTestResults.ql Fixed
@d10c
d10cforce-pushed the d10c/convert-tests-to-qlref branch from 2aaa656 to c628653CompareJune 20, 2025 13:57
Comment threadgo/ql/test/query-tests/Security/CWE-681/FilterTestResults.ql Fixed
@d10c
d10cforce-pushed the d10c/convert-tests-to-qlref branch from c628653 to 657e782CompareJune 20, 2025 14:05
@d10c
d10c marked this pull request as ready for review June 20, 2025 14:30
@d10c
d10c requested review from a team as code ownersJune 20, 2025 14:30
geoffw0
geoffw0 previously approved these changes Jun 23, 2025

@geoffw0geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swift changes LGTM. 👍

TestRemoteSource() { this.asExpr().(ApplyExpr).getStaticTarget().getName().matches("source%") }

override string getSourceType() { result = "Test source" }
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test source isn't supported in the new approach but ... doesn't appear to make any difference. All the test cases use String(contentsOf: ...) as their sources anyway.

owen-mc
owen-mc previously approved these changes Jun 23, 2025

@owen-mcowen-mc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go 👍🏻

@hvitvedhvitved left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for doing this.

def foo
def download_tools(installer)
Excon.get(installer[:url]) # $ MISSING: BAD= (requires hash flow)
Excon.get(installer[:url]) # $ MISSING: $Alert (requires hash flow)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $ in front of Alert should be removed.

cmd = params[:cmd]
`#{cmd}`
system(cmd)
cmd = params[:cmd] # $Source

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We typically have a space after the $ marker.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, I missed that when reviewing Go.

@d10c
d10c dismissed stale reviews from owen-mc and geoffw0 via 35a48e7June 24, 2025 12:59
@d10c
d10cforce-pushed the d10c/convert-tests-to-qlref branch from 657e782 to 35a48e7CompareJune 24, 2025 12:59
@d10c

d10c commented Jun 24, 2025

Copy link
Copy Markdown
ContributorAuthor

Rebased with the suggestions applied (including adding a space in $ Alert etc on all the sources). Let's see if CI still likes it.

@geoffw0geoffw0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swift still LGTM.

@d10c
d10c requested a review from hvitvedJune 24, 2025 13:35
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Gono-change-note-requiredThis PR does not need a change noteRubySwift

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@d10c@hvitved@geoffw0@github-advanced-security@owen-mc