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

Use test_data for big objects, add big block for benchmarking #750

Merged
merged 5 commits into from Jan 7, 2022

Conversation

RCasatta
Copy link
Collaborator

In the first two commits I moved some data from source files to the newly introduced test_data dir, including it with include_[str|bytes]! macro.

The second-to-last commit introduces a big block in test_data which is very handy in ser/de benchmark (I used it for #672) because with smaller blocks you may not notice performance improvements.

Since I don't want to pollute the package the last commit excludes the test_data dir from the published package. I think it's fine to do it because dependent packages don't run dependencies tests.

@RCasatta RCasatta changed the title Use test data for big objects, add big block for benchmarking Use test_data for big objects, add big block for benchmarking Jan 3, 2022
@Kixunil
Copy link
Collaborator

Kixunil commented Jan 3, 2022

Light review ack: didn't verify whether the hashes and data match but I think if they didn't it'd be caught by tests.

apoelstra
apoelstra previously approved these changes Jan 3, 2022
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK eb14443

Also did not check that the hashes etc match but I did check that the claimed blocks are actually the right blocks :). include_bytes! is very cool, good find!

@tcharding tcharding mentioned this pull request Jan 5, 2022
@RCasatta
Copy link
Collaborator Author

RCasatta commented Jan 6, 2022

Had to rebase after merging #680.

After rebase I added 247a14f to use big block instead of making one

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 247a14f

I'm really happy with this. Great to be using real blocks rather than ad-hoc contraptions in our testing and benchmarking code.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

tACK 247a14f

(not sure if my machine is screwed up or tests started to take much longer time than previously can't investigate right now)

@apoelstra apoelstra merged commit f332a19 into rust-bitcoin:master Jan 7, 2022
@RCasatta RCasatta deleted the use_test_data branch January 10, 2022 09:53
@RCasatta
Copy link
Collaborator Author

(not sure if my machine is screwed up or tests started to take much longer time than previously can't investigate right now)

Maybe adding too many include_[bytes|str] every time could blow the binaries. I think a solution to this and to the necessity of sharing functions across tests is to have an internal-only test-utils crate which is used as a dev-dependency via path

sanket1729 added a commit that referenced this pull request Dec 1, 2022
962abcc Add serde regression tests (Tobin Harding)

Pull request description:

  Attempts to add regression tests for _all_ types defined in `rust-bitcoin` that implement `Serialize`/`Deserialize`.

  - Add a `tests` directory and implement regression tests in there
  - Use files for input hex and output bincode to reduce source file clutter
  - Copy test block and `include_bytes!` usage from RCasatta's [PR](#750)
  - Uses Kixunil's macro suggested below
  - Adds a single regression test to `util/taproot.rs` for private types

  ## Note to reviewers
  - Uses JSON for opcodes in a separate file (`tests/regression_opcodes.rs`), for all other tests uses bincode.
  - Bypasses the order issue for maps by only serializing maps with a single element - is this correct?

  Fixes #723

ACKs for top commit:
  apoelstra:
    ACK 962abcc
  sanket1729:
    ACK 962abcc. This has been open for a long time. Merging this in the interest of progress.

Tree-SHA512: e34e48e1c56fab5898bc74e7fb867435ed387d828dd3daf0c7d6df8f305e1da6883e91487115ac428618eb7d95bd16aa2cd209ca219684959bc95587ef0b4083
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.

None yet

3 participants