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

chore: add basic load test #2060

Open
wants to merge 1 commit into
base: feat_add_support_for_JSON5/JSONC_config_files
Choose a base branch
from

Conversation

NathanFlurry
Copy link
Member

Changes

Copy link
Member Author

NathanFlurry commented Feb 20, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR introduces a comprehensive k6 load testing framework for WebSocket functionality in Rivet, with configurable parameters and actor management capabilities.

  • In /tests/load/basic/index.js, core WebSocket testing functionality is commented out, leaving only actor creation/destruction - this needs to be uncommented and implemented
  • /tests/load/basic/actor.js lacks proper timeout configuration in waitForHealth() function which could lead to hanging tests
  • /tests/load/basic/rivet_api.js only supports GET/POST methods, limiting test capabilities for DELETE operations
  • /tests/load/basic/rivet_api.js assumes all successful responses return 200 status code which may not handle 201/204 responses correctly
  • In /tests/load/basic/actor.js, WebSocket error handling doesn't include timeout scenarios or connection failures

5 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile

justfile Outdated
Comment on lines 119 to 120
k6-run TEST:
k6 run --verbose tests/load/{{ TEST }}/index.js
Copy link

Choose a reason for hiding this comment

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

style: k6-run command lacks explicit VU and iteration settings - could lead to unintended load on systems. Consider adding configurable parameters.

Comment on lines 27 to 40
//// Get endpoint info
//const port = actor.network.ports.http;
//const actorOrigin = `${port.protocol}://${port.hostname}:${port.port}${port.path ?? ""}`;
//
//// Wait for health check
//const isHealthy = waitForHealth(`${actorOrigin}/health`);
//if (!isHealthy) {
// console.error('Health check failed');
// return;
//}
//
//// Test WebSocket
//const wsUrl = `${actorOrigin}/ws`;
//testWebSocket(wsUrl);
Copy link

Choose a reason for hiding this comment

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

logic: Core WebSocket testing functionality is commented out. Either uncomment and implement the tests, or remove the commented code if it's not needed.

Comment on lines 26 to 39
export function waitForHealth(url) {
let attempts = 0;
const maxAttempts = 30;

while (attempts < maxAttempts) {
const response = http.get(url);
if (response.status === 200) {
return true;
}
sleep(0.1);
attempts++;
}
return false;
}
Copy link

Choose a reason for hiding this comment

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

logic: Missing http import. waitForHealth will fail at runtime.

Suggested change
export function waitForHealth(url) {
let attempts = 0;
const maxAttempts = 30;
while (attempts < maxAttempts) {
const response = http.get(url);
if (response.status === 200) {
return true;
}
sleep(0.1);
attempts++;
}
return false;
}
import { check, sleep } from "k6";
import ws from "k6/ws";
import http from "k6/http";
import { callRivetApi } from "./rivet_api.js";

Comment on lines 56 to 58
socket.on("error", () => {
reject(false);
});
Copy link

Choose a reason for hiding this comment

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

style: Error handler should include error details in rejection for better debugging


// Standard checks
const checks = {
"status is 200": (r) => r.status === 200,
Copy link

Choose a reason for hiding this comment

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

logic: Status check is too strict. Some valid responses may use 201 (Created) or 204 (No Content).

Suggested change
"status is 200": (r) => r.status === 200,
"status is successful": (r) => r.status >= 200 && r.status < 300,

fail(`${path} request failed`);
}

return JSON.parse(response.body);
Copy link

Choose a reason for hiding this comment

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

style: Attempting to parse response body twice - once in check and once in return. Could cache the parsed result.

@MasterPtato MasterPtato force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from 52486f4 to dc6cea5 Compare February 20, 2025 22:50
@MasterPtato MasterPtato force-pushed the feat_add_basic_websocket_load_testing branch from 7171d0f to c7e13e3 Compare February 20, 2025 22:50
Copy link

cloudflare-workers-and-pages bot commented Feb 20, 2025

Deploying rivet with  Cloudflare Pages  Cloudflare Pages

Latest commit: a4e4261
Status:🚫  Build failed.

View logs

Copy link

cloudflare-workers-and-pages bot commented Feb 21, 2025

Deploying rivet-hub with  Cloudflare Pages  Cloudflare Pages

Latest commit: a4e4261
Status: ✅  Deploy successful!
Preview URL: https://dd7ee1d3.rivet-hub-7jb.pages.dev
Branch Preview URL: https://feat-add-basic-websocket-loa.rivet-hub-7jb.pages.dev

View logs

@MasterPtato MasterPtato force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from c65e1d9 to 97074b7 Compare February 25, 2025 02:47
@MasterPtato MasterPtato force-pushed the feat_add_basic_websocket_load_testing branch from c3e4d57 to 198ece0 Compare February 25, 2025 02:47
@NathanFlurry NathanFlurry force-pushed the feat_add_basic_websocket_load_testing branch from 198ece0 to 8c36eb9 Compare February 26, 2025 01:05
@NathanFlurry NathanFlurry force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from 97074b7 to 2b00537 Compare February 26, 2025 01:05
@MasterPtato MasterPtato force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from 2b00537 to a91972e Compare February 26, 2025 03:29
@MasterPtato MasterPtato force-pushed the feat_add_basic_websocket_load_testing branch from 8c36eb9 to a4e4261 Compare February 26, 2025 03:29
@NathanFlurry NathanFlurry force-pushed the feat_add_basic_websocket_load_testing branch from a4e4261 to 8c36eb9 Compare February 26, 2025 06:18
@NathanFlurry NathanFlurry force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from 2b00537 to 07555f6 Compare February 27, 2025 00:51
@NathanFlurry NathanFlurry force-pushed the feat_add_basic_websocket_load_testing branch from 8c36eb9 to 825a831 Compare February 27, 2025 00:51
@MasterPtato MasterPtato force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from 07555f6 to a91972e Compare February 27, 2025 02:44
@MasterPtato MasterPtato force-pushed the feat_add_basic_websocket_load_testing branch from 825a831 to a4e4261 Compare February 27, 2025 02:44
@NathanFlurry NathanFlurry force-pushed the feat_add_basic_websocket_load_testing branch from a4e4261 to 8c36eb9 Compare February 27, 2025 07:59
@NathanFlurry NathanFlurry force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from a91972e to 2b00537 Compare February 27, 2025 07:59
@MasterPtato MasterPtato force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from 2b00537 to a91972e Compare February 27, 2025 18:57
@MasterPtato MasterPtato force-pushed the feat_add_basic_websocket_load_testing branch from 8c36eb9 to a4e4261 Compare February 27, 2025 18:57
@NathanFlurry NathanFlurry force-pushed the feat_add_basic_websocket_load_testing branch from a4e4261 to 8c36eb9 Compare February 27, 2025 20:46
@MasterPtato MasterPtato force-pushed the feat_add_support_for_JSON5/JSONC_config_files branch from 2b00537 to a91972e Compare February 28, 2025 03:07
@MasterPtato MasterPtato force-pushed the feat_add_basic_websocket_load_testing branch from 8c36eb9 to a4e4261 Compare February 28, 2025 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant