Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS: Port three tests to use the new post processing-based inline test expectations #18472

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jan 10, 2025

Ports three of our .qlref tests to use inline test expectations based on the post-processing (see #17548)

This was mainly to get my hands dirty with this system in preparation for more thorough test suite update. I've verified that these changes don't conflict with the data flow migration PR as the queries don't use data flow.

The PR is structured as follows (and a larger PR might follow a similar structure, possibly with some automation):

  • For each query:
    • Do a direct translation of // OK-style comment to // $ Alert-style and commit the new .expected file, which usually contains some discrepancies.
    • Update inline expectations with MISSING/SPURIOUS and a TODO until there are no discrepancies. The .expected file should contain no discrepancies at this point. (I skipped this in one case)
    • Fix bugs in the test file and/or move comments to their correct location
    • For discrepancies that reflect actual query bugs, remove the TODO and leave the MISSING/SPURIOUS marker

The first commit is a bugfix in the framework that makes it recognise the problems result set as an alias for #select. @hvitved perhaps you would like to review that one?

@github-actions github-actions bot added the JS label Jan 10, 2025
@asgerf asgerf added the no-change-note-required This PR does not need a change note label Jan 10, 2025
@hvitved
Copy link
Contributor

hvitved commented Jan 10, 2025

The first commit is a bugfix in the framework that makes it recognise the problems result set as an alias for #select. @hvitved perhaps you would like to review that one?

Looks good to me.

@asgerf asgerf marked this pull request as ready for review January 10, 2025 14:11
@Copilot Copilot bot review requested due to automatic review settings January 10, 2025 14:11
@asgerf asgerf requested a review from a team as a code owner January 10, 2025 14:11

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 9 changed files in this pull request and generated no comments.

Files not reviewed (6)
  • javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.expected: Language not supported
  • javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/IncompleteHostnameRegExp.qlref: Language not supported
  • javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.expected: Language not supported
  • javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck/IncompleteUrlSchemeCheck.qlref: Language not supported
  • javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSubstringSanitization/IncompleteUrlSubstringSanitization.qlref: Language not supported
  • shared/util/codeql/util/test/InlineExpectationsTest.qll: Language not supported
Comments suppressed due to low confidence (5)

javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js:18

  • The change introduces a new regex pattern that is not covered by tests. Ensure that this pattern is tested to verify its correctness.
new RegExp(`test.example.com$`); // $ MISSING: Alert

javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js:37

  • The change introduces a new regex pattern that is not covered by tests. Ensure that this pattern is tested to verify its correctness.
/^(.+\.(?:example-a|example-b)\.com)\//; // $ MISSING: Alert

javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js:52

  • The change introduces a new regex pattern that is not covered by tests. Ensure that this pattern is tested to verify its correctness.
new RegExp('test.' + primary); // $ MISSING: Alert

javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js:60

  • [nitpick] The use of whatever in the regex pattern is ambiguous. Consider using a more descriptive placeholder to avoid confusion.
/^(foo.example\.com|whatever)$/; // $ Alert (but kinda OK - one disjunction doesn't even look like a hostname)

javascript/ql/test/query-tests/Security/CWE-020/IncompleteHostnameRegExp/tst-IncompleteHostnameRegExp.js:61

  • [nitpick] The use of test.example.com should be consistent with other test cases to avoid confusion. Consider using a more descriptive placeholder.
if (s.matchAll('^http://test.example.com')) {} // $ Alert

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

Nice structure.
LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS 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