Skip to content

refactor: redefine useAgentLogs tests as unit tests #18019

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

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

Conversation

Parkreiner
Copy link
Member

@Parkreiner Parkreiner commented May 23, 2025

Closes coder/internal#644

Changes made

  • Redefined useAgentLogs to support dependency injection for how the websocket connection is created
  • Redefined the test suite for useAgentLogs to work as a set of unit tests
  • Moved our mock websocket logic into a general test helper file
  • Updated the test helper for generating logs so that each log is guaranteed to have a different timestamp

Notes

  • This PR doesn't add any additional test cases, but with the new logic in place, it should be easy to add a lot more
    • If I had more time today, I would've added a test to assert that logs are sorted by timestamp before returning, since websocket messages aren't guaranteed to be in order

@Parkreiner Parkreiner requested a review from BrunoQuaresma May 23, 2025 20:28
@Parkreiner Parkreiner self-assigned this May 23, 2025
@@ -0,0 +1,135 @@
import type { WebSocketEventType } from "utils/OneWayWebSocket";

export type MockWebSocketPublisher = Readonly<{
Copy link
Member Author

Choose a reason for hiding this comment

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

The entire contents of the file were basically copy-pasted from the other test file. The only change was that the publisher type is now exported and was renamed to be a little more clear

Comment on lines +18 to +22
// Make sure that the logs generated each have unique timestamps, so
// that we can test whether they're being sorted properly before being
// returned by the hook
const logDate = new Date(baseDate.getTime() + i * millisecondsInOneMinute);
return {
Copy link
Member Author

@Parkreiner Parkreiner May 23, 2025

Choose a reason for hiding this comment

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

Like I said in the PR body, I didn't add a test to assert the sorting functionality yet, but I updated the test helper anyway, so that I can make sure the old/new sets of logs created in the test case are guaranteed to be different, and there's no risk of false positives

@Parkreiner Parkreiner changed the title refactor: update useAgentLogs tests as unit tests refactor: redefine useAgentLogs tests as unit tests May 23, 2025
@BrunoQuaresma
Copy link
Collaborator

I have to put more time to review this, but I'm starting to think it adds more complex than value to be honest 🤔

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.

flake: useAgentLogs
2 participants