Skip to content

C++: Fill gaps in query tests (part 1) - #22727

Merged
geoffw0 merged 5 commits into
github:mainfrom
geoffw0:qualitytests
Oct 2, 2026
Merged

geoffw0 merged 5 commits into
github:mainfrom
geoffw0:qualitytests

Conversation

@geoffw0

@geoffw0 geoffw0 commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

Fill gaps in CPP query tests, with quality queries in mind:

  • add a test case from cpp/comparison-of-identical-expressions (PointlessSelfComparison.ql) to the test for cpp/constant-comparison (PointlessComparison.ql); establishes no significant overlap in results.
  • add tests for cpp/empty-if (FutileConditional.ql).
  • add test cases from cpp/empty-if (FutileConditional.ql) to the test for cpp/empty-block (EmptyBlock.ql); establishes significant overlap in results.

@geoffw0 geoffw0 added the no-change-note-required This PR does not need a change note label Oct 1, 2026
@geoffw0
geoffw0 requested a review from a team as a code owner October 1, 2026 14:19
Copilot AI balanced review requested due to automatic review settings October 1, 2026 14:19

Copilot AI 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.

Copilot review overview

🟢 Approval recommended

The focused test additions correctly exercise the intended query behavior and overlap boundaries.

Review effort: Balanced
Findings: None

What changed in this PR

Adds C++ query coverage to clarify overlap between related quality queries.

Changes:

  • Adds tests for cpp/empty-if.
  • Verifies overlap with cpp/empty-block.
  • Confirms self-comparisons are not reported by cpp/constant-comparison.
File Description
FutileConditional/​FutileConditional.qlref Configures the new query test.
FutileConditional/​FutileConditional.expected Records expected empty-if alerts.
FutileConditional/​FutileConditional.cpp Adds positive, negative, and known-missing cases.
PointlessComparison/​PointlessComparison.cpp Adds a non-overlap self-comparison case.
EmptyBlock/​EmptyBlock.expected Records additional empty-block results.
EmptyBlock/​empty_block.cpp Adds conditional branch overlap cases.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions github-actions Bot added the C++ label Oct 1, 2026
jketema
jketema previously approved these changes Oct 2, 2026

@jketema jketema 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.

LGTM. Two small comments, but it's fine with me to merge as-is.

}
}

// BAD

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.

Not for this PR, but did we forget to convert this test to an inline expectation test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This one's actually a pain to convert, because comments affect what is and isn't considered an empty block by the query. I decided it's best to leave it as not inline expectations.

@jketema jketema Oct 2, 2026 •

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, ouch, yeah you're right.

Comment on lines +7 to +11
if (value) { // good
++value;
}

if (value) { // good

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: Do we want to capitalize "good" here to be consistent with what we do elsewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated.

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.

Approved.

@geoffw0
geoffw0 merged commit f7617ab into github:main Oct 2, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants