Skip to content

Conversation

Harsh1925
Copy link
Contributor

This PR adds additional unit tests to cover pseudo BigInt parsing across
non-decimal bases. It addresses the in-file TODO in
internal/jsnum/pseudobigint_test.go ("tests for other bases").

What’s included

  • Binary literals (0b/0B), with and without underscores
  • Octal literals (0o/0O), with and without underscores
  • Hex literals (0x/0X), with and without underscores
  • Small number cases (0, 1, 10, 255, etc.), complementing the existing
    large literal tests

Why

  • Improves coverage for ParsePseudoBigInt
  • Verifies parser correctly handles base prefixes, separators, and case
    sensitivity
  • No behavior changes; tests pass with current implementation

@Harsh1925
Copy link
Contributor Author

@microsoft-github-policy-service agree

Copy link
Member

Choose a reason for hiding this comment

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

I do not know why you are modifying this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I added these cases because of the inline TODO in internal/jsnum/pseudobigint_test.go (“tests for other bases”). The TODO I was responding to is in internal/jsnum/pseudobigint_test.go near the “strip base-10 strings” section (just above the “can parse large literals” test).
The file already covers one large literal per base, but didn’t have small/underscore/uppercase-prefix cases, so I added those to complement the existing “large literals” tests.
If you’d prefer these live in a different package/file, or you don’t want to expand this test, I’m happy to move them (or close). Where would you like these cases to go?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know what you're talking about, I'm commenting on the readme

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. The README change slipped in by mistake on this branch.
I opened a clean PR with only the tests: #1678.
Closing this one to keep the diff focused.

@Harsh1925 Harsh1925 closed this Sep 4, 2025
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.

2 participants