Skip to content

Ruby: Address testFailures in inline expectations tests (part 2)#22081

Open
geoffw0 wants to merge 7 commits into
github:mainfrom
geoffw0:rubyinline2
Open

Ruby: Address testFailures in inline expectations tests (part 2)#22081
geoffw0 wants to merge 7 commits into
github:mainfrom
geoffw0:rubyinline2

Conversation

@geoffw0

@geoffw0 geoffw0 commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Address further testFailures in inline expectations tests. Annotations should always be updated so that the testFailures section of the .expected file is empty.

The improper_memoization.rb change is trivial. The other one is in a .erb file, so I had to add support for inline expectations comments in .erb files in order to make the testFailures result go away. I'm fairly happy with that aspect of the PR.

I'd appreciate confirmation that the taint flow result in the .erb is a valid result, and shouldn't be marked as SPURIOUS:. It looks like that's the intent, but I wasn't 100% confident what was going on.

@geoffw0 geoffw0 added the no-change-note-required This PR does not need a change note label Jun 29, 2026
@geoffw0 geoffw0 requested a review from a team as a code owner June 29, 2026 09:54
Copilot AI review requested due to automatic review settings June 29, 2026 09:54
@geoffw0 geoffw0 added the Ruby label Jun 29, 2026

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.

Pull request overview

This PR fixes remaining failures in Ruby inline-expectations-based tests by (1) updating test annotations to match intended outcomes and (2) extending the Ruby inline expectations implementation so expectation markers can be written in .erb template comments.

Changes:

  • Mark the improper_memoization.rb m14 result as a known false positive (SPURIOUS) to eliminate an “Unexpected result” test failure.
  • Add support for parsing inline expectation markers from ERB comments (<%# ... %>) in the Ruby inline expectations test implementation.
  • Update the Sinatra ERB view test to annotate the expected taint flow on the relevant line, clearing the .expected file testFailures section.
Show a summary per file
File Description
ruby/ql/test/query-tests/experimental/improper-memoization/ImproperMemoization.expected Removes a testFailures entry now that the test source is annotated appropriately.
ruby/ql/test/query-tests/experimental/improper-memoization/improper_memoization.rb Adds an inline SPURIOUS annotation for m14 on the end line.
ruby/ql/test/library-tests/frameworks/sinatra/views/index.erb Adds an ERB comment inline expectation for hasTaintFlow on the sink line.
ruby/ql/test/library-tests/frameworks/sinatra/Flow.expected Removes the testFailures entry for the ERB taint flow result.
ruby/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll Extends Ruby inline expectations comment handling to include ERB comment tokens.

Review details

  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread ruby/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Comment thread ruby/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll Outdated

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

One more small suggestion.

Comment thread ruby/ql/lib/utils/test/internal/InlineExpectationsTestImpl.qll
Co-authored-by: Tom Hvitved <hvitved@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Ruby

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants