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

[plain_hasher] Migrate to 2018 edition #213

Merged
merged 6 commits into from
Sep 5, 2019
Merged

[plain_hasher] Migrate to 2018 edition #213

merged 6 commits into from
Sep 5, 2019

Conversation

koushiro
Copy link
Contributor

@koushiro koushiro commented Sep 4, 2019

What I have changed

Signed-off-by: koushiro <koushiro.cqx@gmail.com>
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
Copy link
Member

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thanks! Is there any benefit of running cargo test --benches instead of cargo check --benches?

.travis.yml Outdated
@@ -34,7 +34,7 @@ script:
fi
- cd fixed-hash/ && cargo test --all-features && cd ..
- cd uint/ && cargo test --features=std,quickcheck --release && cd ..
- cd plain_hasher/ && cargo test --no-default-features && cd ..
- cd plain_hasher/ && cargo test --no-default-features && cargo test --benches && cd ..
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- cd plain_hasher/ && cargo test --no-default-features && cargo test --benches && cd ..
- cd plain_hasher/ && cargo test --no-default-features && cargo check --benches && cd ..

Copy link
Contributor Author

@koushiro koushiro Sep 4, 2019

Choose a reason for hiding this comment

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

It just my personal convention. cargo test --benches is building all benchmarkable targets and running them as tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

cargo test --benches runs tests as well. And it seems it actually runs the benches. So that's not what we want here right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it runs the benchmarks like that (include running unit-tests):

running 1 test
test tests::it_works ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

...

Testing write_plain_hasher
Success

Testing write_default_hasher
Success

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, and we do not wish to run the benchmarks on CI.

(Not that it's a bad thing per se to run the benches, but it's not worth anything if we don't collect the output performance data and store it somewhere, and we're not so for the time being running the benches just slows CI down for no benefit.)

Signed-off-by: koushiro <koushiro.cqx@gmail.com>
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@ordian ordian requested a review from dvdplm September 4, 2019 11:04
@ordian
Copy link
Member

ordian commented Sep 4, 2019

@koushiro please merge with master

Signed-off-by: koushiro <koushiro.cqx@gmail.com>
.travis.yml Outdated
@@ -34,7 +34,7 @@ script:
fi
- cd fixed-hash/ && cargo test --all-features && cd ..
- cd uint/ && cargo test --features=std,quickcheck --release && cd ..
- cd plain_hasher/ && cargo test --no-default-features && cd ..
- cd plain_hasher/ && cargo test --no-default-features && cargo test --benches && cd ..
Copy link
Contributor

Choose a reason for hiding this comment

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

cargo test --benches runs tests as well. And it seems it actually runs the benches. So that's not what we want here right?

Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@ordian ordian merged commit 3147d8e into paritytech:master Sep 5, 2019
ordian added a commit that referenced this pull request Sep 5, 2019
* master:
  [plain_hasher] Migrate to 2018 edition (#213)
  [ethbloom] Improve ethbloom (#215)
  [rlp] fix nested unbounded lists (#203)
  stabilize parity-bytes in no_std environment (#212)
  Speed up hex serialization, support Serde `with`, and fix warnings (#208)
  [ethbloom, ethereum-types,kvdb] migrate to 2018 edition (#205)
  Introduce `ContractAddress` newtype instead of scheme enum (#200)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants