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

Add support for expected objects to roundtrip test #69

Merged

Conversation

overhacked
Copy link
Collaborator

Add optional support to test the deserialized bytes against an expected static object. Uses a macro to concisely list modules that contain an expected() function that returns the expected Request or Response object.

Existing tests could have expected objects added to improve the error reporting in case of test failure. Tests from the proto::message module have been moved to tests/roundtrip because they were simply round-trip tests with an additional check against an expected object.

@overhacked overhacked force-pushed the overhacked/roundtrip-test-expected-object branch from 93e86cc to 4494e5a Compare May 11, 2024 11:49
@overhacked
Copy link
Collaborator Author

Something weird here. I've run fmt twice and CI keeps failing on fmt --check. Will investigate today.

@wiktor-k
Copy link
Owner

You need a nightly formatter: cargo +nightly fmt for import reordering.

@overhacked
Copy link
Collaborator Author

🙃 I have been running cargo +nightly fmt, but I ran into a rustfmt bug, I think. I 1) ran fmt, 2) commit --amend the changes, then 3) push. CI fmt --check failed, and I discovered that if I ran cargo +nightly fmt again on the code I had committed, it had additional lints applied.

In this case, I had multiple trailing blank lines in a doc comment, and it was only stripping them off one line at a time. The next force-push I make should pass fmt --check.

@overhacked overhacked force-pushed the overhacked/roundtrip-test-expected-object branch from 4494e5a to 47f40e3 Compare May 11, 2024 13:58
@overhacked
Copy link
Collaborator Author

I ran into a rustfmt bug, I think.

Turned out to be true and submitted a fix: rust-lang/rustfmt#6163

@wiktor-k
Copy link
Owner

Wow, crazy! I didn't have time to look deeper in the code but it's cool that you've found a bug and submitted a fix PR 👏

@jcspencer
Copy link
Collaborator

These look great! Once this is merged I'm happy to go back and implement the same for the OpenSSH extensions we have built in, as they also implement their own roundtrip test logic; much nicer to make it more unified!

@overhacked overhacked force-pushed the overhacked/roundtrip-test-expected-object branch from 47f40e3 to 3200b8d Compare May 12, 2024 03:02
@overhacked
Copy link
Collaborator Author

Just broke this PR up into two separate commits to, hopefully, make it easier to review:

  • The first commit contains the changes to tests/roundtrip infrastructure to support checking the deserialized object against an expected, statically-constructed object.
  • The second commit is the conversion of proto::message unit tests to roundtrip tests, utilizing the new expected object support.
  • (The third commit is a documentation fix to the first commit.)

@wiktor-k
Copy link
Owner

Okay, I've read it. I didn't expect it but it's an clever solution and the line-count is good so let's go ahead and merge it 🎉

Could you rebase on top of main? (If there are any conflicts it's because of my PRs and I'm happy to help resolve 😅 )

Add optional support to test the deserialized bytes against an expected
static object. Uses a macro to concisely list modules that contain an
`expected()` function that returns the expected `Request` or `Response`
object.

Existing tests could have expected objects added to improve the error
reporting in case of test failure.

Signed-off-by: Ross Williams <[email protected]>
Move tests from the `proto::message` module to `tests/roundtrip` because
they were simply round-trip tests with an additional check against an
expected object. Use new "expected object" support to test the
deserialization against a statically-initialized object, just as the
unit tests did.

Signed-off-by: Ross Williams <[email protected]>
Usage examples had wrong syntax

Signed-off-by: Ross Williams <[email protected]>
@overhacked overhacked force-pushed the overhacked/roundtrip-test-expected-object branch from 3200b8d to 65f8481 Compare May 14, 2024 23:45
@overhacked
Copy link
Collaborator Author

Rebased onto main; should be good to go!

Copy link
Owner

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

Great! Thank you! 🙇‍♂️

@wiktor-k wiktor-k merged commit 845c218 into wiktor-k:main May 15, 2024
16 checks passed
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