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.

Sign up for GitHub

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

Jump to bottom

encode_unsigned_1 #676

Open
wants to merge 17 commits into
base: develop
Choose a base branch
from
Open

encode_unsigned_1 #676

wants to merge 17 commits into from

Conversation

mlwilkerson
Copy link
Contributor

@mlwilkerson mlwilkerson commented Nov 2, 2020 •
edited

closes #671

  • test boundary case for smallest bigint
  • add proptest for non-integer
  • add proptest for negative integer
  • fix bug with dropping middle zeros
  • fix bug with BigInt with trailing zeros
  • migrate unit tests to integration tests

KronicDeth
KronicDeth requested changes Nov 2, 2020
View reviewed changes
Copy link
Collaborator

@KronicDeth KronicDeth left a comment

Choose a reason for hiding this comment

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

Switch the unit tests to integration tests

native_implemented/otp/src/erlang/encode_unsigned_1.rs Outdated
@@ -0,0 +1,58 @@
#[cfg(all(not(target_arch = "wasm32"), test))]
mod test;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're moving away from unit tests and preferring writing integration tests in Erlang now under native_implemented/otp/tests/internal/lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, does that also mean that we're moving away from proptest as well?

How do I run these integration tests in local dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, for now. I think I figured out how to wire up and run the erlang integration tests.

@mlwilkerson mlwilkerson marked this pull request as ready for review November 2, 2020 22:24
@mlwilkerson
Copy link
Contributor Author

I've migrated the tests to the integration testing pattern. Should the unit tests be removed entirely? That'd result in a loss of some coverage from the proptests, at least.

Also, when doing these as integration tests, all of the ones that use big integer inputs are failing with badarg when I wouldn't expect them to--and they don't fail in the unit tests. This suggests that something higher up in the stack is balking at these big integers, but I haven't investigated that.

@bcardarella
Copy link

FYI I'll ask about this PR in today's standup

KronicDeth
KronicDeth requested changes Mar 31, 2021
View reviewed changes
Copy link
Collaborator

@KronicDeth KronicDeth left a comment

Choose a reason for hiding this comment

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

Three tests are failing for me:


---- lib::binary::encode_unsigned_1::when_big_int_encoded_bytes_have_significant_trailing_zeros stdout ----
thread 'lib::binary::encode_unsigned_1::when_big_int_encoded_bytes_have_significant_trailing_zeros' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"<<64,0,0,0,0,0>>\n"`: 
Commands:
cd /Users/luke.imhoff/github/lumen/lumen/native_implemented/otp
"/Users/luke.imhoff/github/lumen/lumen/native_implemented/otp/tests/_build/internal/lib/binary/encode_unsigned_1/when_big_int_encoded_bytes_have_significant_trailing_zeros/bin/when_big_int_encoded_bytes_have_significant_trailing_zeros"
stdout: 
stderr: Backtrace (most recent call last):
  File /Users/luke.imhoff/github/lumen/lumen/native_implemented/otp/src/binary/encode_unsigned_1.rs:13, in binary:encode_unsigned/1

Process (#PID<0.2.0>) exited abnormally.
  badarg


Status code: 0
Signal: ', native_implemented/otp/tests/internal/lib/binary/encode_unsigned_1.rs:11:1
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- lib::binary::encode_unsigned_1::when_big_int_encoded_bytes_have_zeros_in_the_middle stdout ----
thread 'lib::binary::encode_unsigned_1::when_big_int_encoded_bytes_have_zeros_in_the_middle' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"<<64,0,0,0,0,1>>\n"`: 
Commands:
cd /Users/luke.imhoff/github/lumen/lumen/native_implemented/otp
"/Users/luke.imhoff/github/lumen/lumen/native_implemented/otp/tests/_build/internal/lib/binary/encode_unsigned_1/when_big_int_encoded_bytes_have_zeros_in_the_middle/bin/when_big_int_encoded_bytes_have_zeros_in_the_middle"
stdout: 
stderr: Backtrace (most recent call last):
  File /Users/luke.imhoff/github/lumen/lumen/native_implemented/otp/src/binary/encode_unsigned_1.rs:13, in binary:encode_unsigned/1

Process (#PID<0.2.0>) exited abnormally.
  badarg


Status code: 0
Signal: ', native_implemented/otp/tests/internal/lib/binary/encode_unsigned_1.rs:23:1

---- lib::binary::encode_unsigned_1::with_smallest_big_int stdout ----
thread 'lib::binary::encode_unsigned_1::with_smallest_big_int' panicked at 'assertion failed: `(left == right)`
  left: `""`,
 right: `"<<64,0,0,0,0,0>>\n"`: 
Commands:
cd /Users/luke.imhoff/github/lumen/lumen/native_implemented/otp
"/Users/luke.imhoff/github/lumen/lumen/native_implemented/otp/tests/_build/internal/lib/binary/encode_unsigned_1/with_smallest_big_int/bin/with_smallest_big_int"
stdout: 
stderr: Backtrace (most recent call last):
  File /Users/luke.imhoff/github/lumen/lumen/native_implemented/otp/src/binary/encode_unsigned_1.rs:13, in binary:encode_unsigned/1

Process (#PID<0.2.0>) exited abnormally.
  badarg


Status code: 0
Signal: ', native_implemented/otp/tests/internal/lib/binary/encode_unsigned_1.rs:2:1


failures:
    lib::binary::encode_unsigned_1::when_big_int_encoded_bytes_have_significant_trailing_zeros
    lib::binary::encode_unsigned_1::when_big_int_encoded_bytes_have_zeros_in_the_middle
    lib::binary::encode_unsigned_1::with_smallest_big_int

test result: FAILED. 5 passed; 3 failed; 0 ignored; 0 measured; 473 filtered out; finished in 14.28s

     Running target/debug/deps/test-08005e5a54fe292e

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass '-p liblumen_otp --test internal'
[cargo-make] ERROR - Error while executing command, exit code: 101
[cargo-make] WARN - Build Failed.

Since those involve large big int literals it could be something with the literal encoding in the compile. If that's the case, there's nothing for you to do @mlwilkerson and we'll have to fix that bug. Might be able to work around it with shifts.

@bitwalker bitwalker force-pushed the develop branch 3 times, most recently from d92d24a to 7da47da Compare August 31, 2022 22:43
@bitwalker bitwalker force-pushed the develop branch 4 times, most recently from 8d0dd93 to 213b3f3 Compare February 27, 2023 07:11
@bitwalker bitwalker force-pushed the develop branch 2 times, most recently from a6a14cb to 6d51d5c Compare February 27, 2023 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KronicDeth KronicDeth KronicDeth requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

binary:encode_unsigned/1 BIF
3 participants
@mlwilkerson @bcardarella @KronicDeth