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

test(js/internal): add unit tests for Dequeue #16189

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

Conversation

DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Jan 6, 2025

What does this PR do?

These changes are being separated out from an in-progress PR I'm working on.

  • Add unit tests for Dequeue internal
  • Add type annotations to Dequeue
  • Add some comments to previously-TODO builtins

How did you verify your code works?

I've added tests

@DonIsaac DonIsaac requested a review from paperclover January 6, 2025 22:43
Copy link
Member

@paperclover paperclover left a comment

Choose a reason for hiding this comment

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

seems reasonable to merge, but this doesn't seem to fix/catch any bugs yet(??)

see nitpicks

test/internal/fifo.test.ts Outdated Show resolved Hide resolved
test/internal/fifo.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

DonIsaac commented Jan 6, 2025

@DonIsaac
Copy link
Contributor Author

DonIsaac commented Jan 6, 2025

@paperclover correct, not yet; that's what the other PR is doing

@robobun
Copy link

robobun commented Jan 7, 2025

Updated 3:02 AM PT - Jan 8th, 2025

@DonIsaac, your commit 911b2e1 has 23 failures in #9227:


🧪   try this PR locally:

bunx bun-pr 16189

@DonIsaac DonIsaac enabled auto-merge January 7, 2025 18:40
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.

3 participants