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

Fixing random CI failures due to message skips #74

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

aryanjassal
Copy link
Member

@aryanjassal aryanjassal commented Dec 2, 2024

Description

This PR focuses on resolving #70, which is a critical bug that happens in the CI because the second message is skipped.

Issues Fixed

Tasks

  • 1. Fix the error
  • 2. Run all tests to ensure the error isn't present
  • 3. Change the method from cancelling and attaching streams to instead be more dynamic like returning a promise for the header.

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@aryanjassal aryanjassal self-assigned this Dec 2, 2024
Copy link

linear bot commented Dec 2, 2024

tests/RPCServer.test.ts Outdated Show resolved Hide resolved
@aryanjassal
Copy link
Member Author

I removed a couple tests from the files as they were redundant and literally duplicating code. Some others, I feel like they might be redundant. I will be going through the changes in-depth tomorrow and make a brief list discussing the reasoning behind the test removals.

Also, the formatting, while correct, was too verbose. I re-formatted the code, which is the main culprit in adding a lot of lines changed in this commit.

However, in conclusion, basically everything is working at this point, and the weird stream plumbing has been replaced with a more elegant solution as a special transform stream which returns the JSON header message after parsing and returns the following as raw bytes without applying any processing on it.

The CI for this change is passing, both locally and on github. The only way to see if the change is actually made and is consistent is to merge it to staging and see whether the CI breaks or not.

Also, @brynblack still needs to set up auto deployment for this repo. Maybe after I have merged this PR and (hopefully) fixed the CI, you can merge a quick fix for that.

@CMCDragonkai
Copy link
Member

This is a blocker, please address this asap @brynblack.

Copy link
Member Author

@aryanjassal aryanjassal left a comment

Choose a reason for hiding this comment

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

Just need some general inputs on some things.

tests/RPCClient.test.ts Show resolved Hide resolved
tests/RPCClient.test.ts Outdated Show resolved Hide resolved
tests/RPCClient.test.ts Outdated Show resolved Hide resolved
tests/RPCServer.test.ts Show resolved Hide resolved
tests/RPCServer.test.ts Outdated Show resolved Hide resolved
src/RPCServer.ts Outdated Show resolved Hide resolved
src/RPCServer.ts Outdated Show resolved Hide resolved
src/RPCServer.ts Show resolved Hide resolved
src/middleware.ts Outdated Show resolved Hide resolved
src/middleware.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
tests/RPCClient.test.ts Outdated Show resolved Hide resolved
chore: fixed ci and updated all tests

chore: cleaned up code

chore: moved common function out
@aryanjassal aryanjassal merged commit 22c79e1 into staging Dec 6, 2024
8 checks passed
@CMCDragonkai
Copy link
Member

Did this change any expected behaviour at the UI level?

@aryanjassal
Copy link
Member Author

Did this change any expected behaviour at the UI level?

No, there was no external change to using the streams in any way. This PR just changed internal code related to retrieval of the header message to use a more elegant system instead of replumbing streams.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Transferring data through the RPC stream skips the second message
3 participants