Skip to content

Fix rand_os warning in tests#144

Merged
str4d merged 2 commits into
zcash:masterfrom
str4d:warning-fix
Nov 4, 2019
Merged

Fix rand_os warning in tests#144
str4d merged 2 commits into
zcash:masterfrom
str4d:warning-fix

Conversation

@str4d

@str4d str4d commented Oct 31, 2019

Copy link
Copy Markdown
Contributor

Warnings were originally fixed in #134. This warning was introduced in #114, but it wasn't spotted because there was no merge conflict.

@str4d str4d added this to the v0.2.0 milestone Oct 31, 2019

@nathan-at-least nathan-at-least left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am confused:

How can all three of these be true? Shouldn't that use rand_os line cause building tests to fail, since it's not a declared dependency?

@nathan-at-least

Copy link
Copy Markdown
Contributor

Hm, if the tests aren't build/executed for the code being patched, I would change my review to "Request Changes" to request they get added into the CI pipeline in this PR. If instead that code is tested but there's something I'm missing / confused about with dependencies or rust, then I would probably approve this comment and learn something new.

@str4d

str4d commented Oct 31, 2019

Copy link
Copy Markdown
Contributor Author
  • Before merging this PR, it appears that master builds/tests successfully.

The tests all pass on this branch. The one failing step is Travis CI, which is trying to also run code coverage and as a result often runs over the 50-minute maximum execution time. Re-running the build is usually sufficient to have it succeed because then the build step is cached.

This PR is changing the zcash_client_backend crate, not the librustzcash crate. But this raises a good point: I forgot to remove the rand_os dependency from zcash_client_backend. I'll do so now.

@nathan-at-least nathan-at-least left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this resolves my previous confusion, and my new belief is that this dependency was declared, but no longer use'd anywhere. Is that true?

(Seems like an opportunity for a linter / unused-code style warning.)

@str4d

str4d commented Nov 4, 2019

Copy link
Copy Markdown
Contributor Author

I believe this resolves my previous confusion, and my new belief is that this dependency was declared, but no longer use'd anywhere. Is that true?

Correct. It was declared as a dev-dependency though, so zero effect on the main code. The tests wouldn't catch unused dev-dependencies, because they can be used for tests, doc-tests, examples, and benchmarks. There might be a clippy lint for it, but we're a while away from being able to add those to CI.

@Eirik0 Eirik0 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

utACK

@str4d str4d merged commit 720ee64 into zcash:master Nov 4, 2019
@str4d str4d deleted the warning-fix branch November 4, 2019 23:03
greg0x pushed a commit to valargroup/librustzcash that referenced this pull request Mar 12, 2026
…-risk

fix: prevent mock verifiers from silently bypassing crypto in production
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