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 pb-long without BI support and fix found issues #573

Merged
merged 2 commits into from
Jul 31, 2023

Conversation

jcready
Copy link
Contributor

@jcready jcready commented Jul 27, 2023

Initially my goal was simply to improve code coverage, but in doing so I discovered a number of issues related to pb-long when the test environment did not have BigInt support.

Issues discovered and fixed with pb-long w/o BI support:

  1. PbLong.from() did not throw when the string value was too small "-9223372036854775809", "-18446744073709551616"
  2. let maxSignedPlusOneStr = "9223372036854775808"
    expect(() => PbLong.from(maxSignedPlusOneStr)).toThrowError();
    Expected function to throw an Error.
  3. it('should toString() max unsigned int64', function () {
    let uLong = new PbULong(-1, -1);
    expect(uLong.toString()).toBe('18446744073709551615');
    });
    Expected '-1' to be '18446744073709551615'.
  4. it('should toString() min signed int64', function () {
    let uLong = new PbLong(0, -2147483648);
    expect(uLong.toString()).toBe('-9223372036854775808');
    });
    Expected '--9223372036854776000' to be '-9223372036854775808'.

This change fixes those issues, ensures all pb-long tests are performed with and without BI support so the testing runtime environment doesn't strictly limit what can be tested. It also improves the test coverage for pb-long.

Copy link
Owner

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

Pushed up one commit that moves the test setup into a describeWithAndWithoutBISupport() function.

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