-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: main
Are you sure you want to change the base?
Conversation
Looks good to me. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice structure.
LGTM 👍
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):
// OK
-style comment to// $ Alert
-style and commit the new.expected
file, which usually contains some discrepancies.MISSING
/SPURIOUS
and aTODO
until there are no discrepancies. The.expected
file should contain no discrepancies at this point. (I skipped this in one case)TODO
and leave theMISSING
/SPURIOUS
markerThe 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?