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 Polykey and all tests for release #562

Merged
merged 9 commits into from
Oct 9, 2023

Conversation

tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented Oct 6, 2023

Description

This PR addresses fixing all tests and bringing Polykey to a fully working release state.

This will merge after #560 if it is ready and will eventually target it. Right not this is branched off of staging.

Issues Fixed

  • No issues directly fixed here AFAIK, I'll have to check.

Tasks

  • 1. Fix all tests and all domains.
  • 2. Confirm that Polykey is in a fully functional state.

Final checklist

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

@tegefaulkes tegefaulkes self-assigned this Oct 6, 2023
@tegefaulkes tegefaulkes changed the title tests: all nodes domain tests are working Fixing Polykey and all tests for release Oct 6, 2023
@ghost
Copy link

ghost commented Oct 6, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@tegefaulkes
Copy link
Contributor Author

Progress update. All domains are function except for the following failures

  1. vaultManager has some failing tests relating to cloning and pulling. It's all the same error so it should be a single fix.
  2. A single test checking if the globalThis.crypto was monkey patched. The patch was removed due to node20 changes so this test can be removed. Unless we're moving back to node 18 in which case we'll keep it.

Close to done. After fixes I'll review any disabled tests and TODOs.

@tegefaulkes
Copy link
Contributor Author

I think there's a bug in the streams with quic.

In one part of the vaults code, we're accumulating all the raw stream messages into an array of Array<Uint8Array>. the contents of this is wrong.

    [
      <Buffer 30 30 30 30 66 36 33 33 65 65 61 64 33 34 66 37 62 38 65 64 38 61 30 64 64 33 63 30 32 36>,
      <Buffer 30 30 30 30>,
      <Buffer 30 30 30 30 66 36 33 33 65 65 61 64 33 34 66 37 62 38 65 64 38 61 30 64 64 33 63 30 32 36 32 37 65 31 30 36 39 62 39 65 37 61 31 31 20 72 65 66 73 2f ... 98 more bytes>,
      <Buffer 30 30 30 30 66 36 33 33 65 65 61 64 33 34 66 37 62 38 65 64 38 61 30 64 64 33 63 30 32 36 32 37 65 31 30 36 39 62 39 65 37 61 31 31 20 72 65 66 73 2f ... 13 more bytes>,
      <Buffer 30 30 30 30>
    ]

They're the expected lengths but the contents are identical, As if the contents have been overwritten with each message. If I print out the contents of the buffers for each message as it's read then we have the correct contents.

This problem is consistent with the stream re-using the same buffer for every message, thus overwriting the contents.

It seems wild to me that this wasn't caught before. but It seems during normal streaming usage the contents of the message are correct when they're used or in most cases a transform step or processing preforms a copy of the data. This only happened here because we just too the buffers and pushed them to an array. Thus giving it time to overwrite itself.

I'll need to apply a fix to js-quic and review the tests to see why exactly this wasn't caught.

@tegefaulkes
Copy link
Contributor Author

Pretty much done now and ready to merge. I'll rebase this on top of #560 branch and merge it into that.

@tegefaulkes
Copy link
Contributor Author

This has been re-based on top of feature-client-consolidation. I'll change the target here and merge when I think it's clear to do so.

@tegefaulkes tegefaulkes changed the base branch from staging to feature-client-consolidation October 9, 2023 07:04
@tegefaulkes
Copy link
Contributor Author

Looking good, I'm going have a look at #560 now.

Process is being held open for a time still, there is a timer not being cleaned up properly.

[ci skip]
@tegefaulkes
Copy link
Contributor Author

I did some cleaning up in #560 and rebased this on top. I'm going to merge it into #560 and finish off in that PR.

@tegefaulkes tegefaulkes marked this pull request as ready for review October 9, 2023 09:18
@tegefaulkes
Copy link
Contributor Author

Skipping normal checklist, it will be handled in #560

@tegefaulkes tegefaulkes merged commit 8faddb0 into feature-client-consolidation Oct 9, 2023
@tegefaulkes tegefaulkes mentioned this pull request Oct 10, 2023
7 tasks
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.

1 participant