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

utils: add a few more examples and tests to MaybeValidated #5346

Merged
merged 4 commits into from
Nov 23, 2021

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Nov 18, 2021

Extend existing and introduce new examples of MaybeValidated usage
which test the type a bit more extensively.

Extend existing and introduce new examples of MaybeValidated usage
which test the type a bit more extensively.
/// assert_eq!(Ok(true), value_as_ref.validate_with::<(), _>(|&&v| Ok(v == 42)));
/// assert!(value_as_ref.is_validated());
/// assert!(!value.is_validated());
/// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to think that doctests are not worth it for non-library crates in general. The problem with doctests is that they are notoriously slow to compile and run, as each doctest is compiled as a separate binary. For example, in rust-analyzer, removing doctest = false from Cargo.tomls increases the wall-clock time for the testsuite from 5 seconds to 10 seconds, and ra doesn't even have doctets.

OTOH, our tests are already pretty slow, and we dont' add doctest=false to cargo.tomls, so this isn't a regression for nearcore. So, no strong opinion here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don’t have an opinion either way. Wasn’t really aware that there was noticeable compile-time cost. I’m happy to rewrite it as [test].

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, I guess let's leave it as is -- we do seem to have some doctests scattered elsewhere. Although, once #4490 is fixed, it makes sense to quantify how bad are doctests for our case. Maybe by that time rust-lang/rust#75341 is fixed, which would be ideal :)

@near-bulldozer near-bulldozer bot merged commit 097f9c5 into near:master Nov 23, 2021
@mina86 mina86 deleted the maybe-test branch November 23, 2021 23:27
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.

4 participants