Skip to content
New issue

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

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

Already on GitHub? # to your account

JS: Refactor WebSocket to use API graphs #19218

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

Conversation

Napalys
Copy link
Contributor

@Napalys Napalys commented Apr 4, 2025

Refactored WebSocket to use API graphs, thus it would work with custom wrappers.
Added inline test expectations for library-tests/frameworks/WebSocket.

@github-actions github-actions bot added the JS label Apr 4, 2025
@Napalys Napalys force-pushed the js/upgrade_websocket branch from 742d33d to 6bcfd8c Compare April 4, 2025 10:32
@Napalys Napalys marked this pull request as ready for review April 7, 2025 10:02
@Copilot Copilot bot review requested due to automatic review settings April 7, 2025 10:03
@Napalys Napalys requested a review from a team as a code owner April 7, 2025 10:03
Copy link
Contributor

@Copilot Copilot AI left a comment

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 refactors the WebSocket implementation to work with API graphs and includes inline test expectations for various WebSocket modules. Key changes include adding inline test expectation comments to the WebSocket event handlers, refactoring the SockJS connection logic, and exporting WebSocket-related instances for custom test scenarios.

Reviewed Changes

Copilot reviewed 7 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
javascript/ql/test/library-tests/frameworks/WebSocket/sockjs.js Updates the SockJS connection logic and adds inline test expectation tags.
javascript/ql/test/library-tests/frameworks/WebSocket/server.js Refactors the WebSocket server setup by adding inline test expectation tags and exporting the WebSocket server instance.
javascript/ql/test/library-tests/frameworks/WebSocket/server-custom.js Adds a custom server setup using exported WebSocket server data with inline expectation comments.
javascript/ql/test/library-tests/frameworks/WebSocket/client.js Updates the client setup with inline test expectation comments and exports WebSocket client instances.
javascript/ql/test/library-tests/frameworks/WebSocket/client-custom.js Implements a custom client configuration using exported WebSocket client instances with inline expectation comments.
javascript/ql/test/library-tests/frameworks/WebSocket/browser.js Refactors browser-based WebSocket and SockJS usage with inline expectation comments and exports instances.
javascript/ql/test/library-tests/frameworks/WebSocket/browser-custom.js Provides a custom browser-based WebSocket configuration using imported instances and inline expectation tags.
Files not reviewed (4)
  • javascript/ql/lib/semmle/javascript/frameworks/WebSocket.qll: Language not supported
  • javascript/ql/src/Security/trest/test.ql: Language not supported
  • javascript/ql/test/library-tests/frameworks/WebSocket/test.expected: Language not supported
  • javascript/ql/test/library-tests/frameworks/WebSocket/test.qlref: Language not supported
Comments suppressed due to low confidence (1)

javascript/ql/test/library-tests/frameworks/WebSocket/server-custom.js:6

  • [nitpick] Inline test expectation tags should be consistent. Consider removing the extra space to match the '$serverSocket' format used elsewhere.
wss.on('connection', function connection(ws) { // $ serverSocket

Tip: Leave feedback on Copilot's review comments with the 👎 and 👍 buttons to help improve review quality. Learn more

@github github deleted a comment from Copilot bot Apr 7, 2025
@github github deleted a comment from Copilot bot Apr 7, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant