removed std reexports from util && fixed broken tests #6187

debris · 2017-07-29T21:05:02Z

changes:

removed std reexports from util - we were rarely using more than 1-2 imported structs
moved Bloomable trait to it's own crate - bloomable
table and using_queue crates are now dependencies of ethcore, not util
changed test script to actually test all targets, not just arbitrary subset. @General-Beck please review scripts/*.sh and test.sh
fixed all not compiling targets and tests (excluding evmjit and ipfs)

debris · 2017-07-30T09:58:04Z

this pr requires latest cargo

eira-fransham

I've got a few questions but nothing sticks out as egregious. I'd be happy to approve this but I'll keep it as unapproved pending conversation on some of the comments.

eira-fransham · 2017-08-01T09:11:37Z

ethcore/src/engines/authority_round/mod.rs

@@ -924,10 +927,10 @@ mod tests {
 		let addr = tap.insert_account("0".sha3().into(), "0").unwrap();
 		let mut parent_header: Header = Header::default();
 		parent_header.set_seal(vec![encode(&0usize).into_vec()]);
-		parent_header.set_gas_limit(U256::from_str("222222").unwrap());


Any particular reason for this? I'm not fussed, but I was under the impression that these were equivalent?

from_str requires us to import std::str::FromStr. parse does not and is more idiomatic

parse requires FromStr, but I definitely agree that it's more idiomatic.

it does not require the import of FromStr

Ah, true, sorry my brain skipped over that word in your comment

eira-fransham · 2017-08-01T09:12:31Z

dapps/src/api/time.rs

 use futures::{self, Future, BoxFuture};
 use futures_cpupool::CpuPool;
 use ntp;
 use time::{Duration, Timespec};
-use util::{Arc, RwLock};
+use util::RwLock;


This is because it uses parking_lot and not the stdlib RwLock, right?

eira-fransham · 2017-08-01T09:14:31Z

ethcore/src/header.rs

@@ -16,13 +16,13 @@

 //! Block header.

+use std::cmp;
+use std::cell::RefCell;
 use util::*;
 use basic_types::{LogBloom, ZERO_LOGBLOOM};
 use time::get_time;
 use rlp::*;


Probably not for this PR, but it'd be really nice if we could deglob everything apart from util (and ideally, even util). Better RLS refactoring support would make that way easier.

eira-fransham · 2017-08-01T09:31:00Z

ethkey/cli/src/main.rs

@@ -170,7 +170,7 @@ fn main() {
 fn display(keypair: KeyPair, mode: DisplayMode) -> String {
 	match mode {
 		DisplayMode::KeyPair => format!("{}", keypair),
-		DisplayMode::Secret => format!("{:?}", keypair.secret()),
+		DisplayMode::Secret => format!("{}", keypair.secret().to_hex()),
 		DisplayMode::Public => format!("{:?}", keypair.public()),


If we're not using the debug display for Secret, can we remove the debug display for Public and Address too? Since this is for displaying to a user these are a clear misuse of Debug.

I generally agree, but I believe it's not a scope of this pr. Here is why:

Publish and Address are just typenames for H512 and H160 whereas Secret is a wrapper around H256. Recently someone has changed the way Secret is displayed and broke tests. Our CI hasn't catched that, cause it was not running ethkey-cli tests... This change only makes our old tests pass...

Ah, I see. Would be a nice issue to file.

eira-fransham · 2017-08-01T09:31:15Z

ethkey/cli/src/main.rs

-"secret:  17d08f5fe8c77af811caa0c9a187e668ce3b74a99acc3f6d976f075fa8e0be55
-public:  689268c0ff57a20cd299fa60d3fb374862aff565b20b5f1767906a99e6e09f3ff04ca2b2a5cd22f62941db103c0356df1a8ed20ce322cab2483db67685afd124
-address: 26d1ec50b4e62c1d1a40d16e7cacc6a6580757d5".to_owned();
+"secret:  aa22b54c0cb43ee30a014afe5ef3664b1cde299feabca46cd3167a85a57c39f2


Any reason that these strings are different?

Yes. In early version of ethkey there was a bug in brainwallet generation. Those tests were written for bugged version and were never updated, cause the CI never run them

Makes sense

eira-fransham · 2017-08-01T09:32:35Z

ipfs/src/route.rs

@@ -119,7 +119,8 @@ mod tests {
 	use ethcore::client::TestBlockChainClient;

 	fn get_mocked_handler() -> IpfsHandler {
-		IpfsHandler::new(None, None, Arc::new(TestBlockChainClient::new()))
+		//IpfsHandler::new(None, None, Arc::new(TestBlockChainClient::new()))


We have git, we don't need to comment code (I assume this was an accident). Is this not compiling anymore? If it's unimplemented can we remove the function entirely?

Agreed, I should have removed that line.

Yes, it's not compiling, and those tests were not run by our CI. Now this code is being compiled by CI, but the tests are excluded (using --exclude) flag.

I think my preference would be to leave the original code, uncommented. Just keep the code as similar as possible but --exclude it. It doesn't matter really though, whatever you would prefer.

eira-fransham · 2017-08-01T09:33:36Z

scripts/cov.sh

@@ -20,8 +20,7 @@ if ! type $KCOV > /dev/null; then
 	exit 1
 fi

-. ./scripts/targets.sh
-RUSTFLAGS="-C link-dead-code" cargo test $TARGETS --no-run || exit $?
+RUSTFLAGS="-C link-dead-code" cargo test --all --exclude ipfs --exclude evmjit --no-run || exit $?


Why do we exclude IPFS and EVMJIT (same for the doc below)?

They are broken and their tests are failing. Fixing them is out of scope

I will create issues to track this

eira-fransham · 2017-08-01T10:08:11Z

Ok, with the questions answered and the commented code fixed I'd say this LGTM

eira-fransham

LGTM. CI is failing and there's merge conflicts, but all the issues I had are fixed.

debris · 2017-08-01T12:04:02Z

Can't merge now, cause wasm, evm and vm tests are broken by #6184. Waiting for the rescue from @NikVolf

debris added 3 commits July 29, 2017 21:10

util reexports less std

eecd823

removed util::common

c4989dd

alway test --all (temporary exclude ipfs and evmjit)

2b02651

debris added A0-pleasereview 🤓 Pull request needs code review. A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Jul 29, 2017

debris requested a review from rphmeier July 29, 2017 21:05

debris mentioned this pull request Jul 29, 2017

CI does not start rust tests when it should #6188

Closed

debris added 2 commits July 29, 2017 23:19

fixed json_tests

48f28fe

Merge branch 'master' into split

88cc4cd

NikVolf added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jul 31, 2017

debris added 3 commits July 31, 2017 16:19

Merge branch 'master' into split

81b57ce

Merge branch 'master' into split

3c7ba50

fixed tests

bfbeda4

eira-fransham reviewed Aug 1, 2017

View reviewed changes

This was referenced Aug 1, 2017

ipfs tests are not compiling and are not run by CI #6204

Closed

evmjit update #6205

Closed

debris mentioned this pull request Aug 1, 2017

Unify debug/display for Address/Public/Secret #6206

Closed

fixed scripts and reverted ipfs comment

6e5a563

eira-fransham approved these changes Aug 1, 2017

View reviewed changes

debris mentioned this pull request Aug 1, 2017

Decouple virtual machines #6184

Merged

Merge branch 'master' into split

a2c0512

debris force-pushed the split branch from 7e352d9 to 0f5d5e7 Compare August 1, 2017 13:07

fix for util on linux

af8c5bb

debris force-pushed the split branch from 0f5d5e7 to af8c5bb Compare August 1, 2017 13:53

arkpar added A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 1, 2017

Fixed evmbin

7f85c0c

debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. labels Aug 1, 2017

debris merged commit b24053f into master Aug 1, 2017

debris deleted the split branch August 1, 2017 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

removed std reexports from util && fixed broken tests #6187

removed std reexports from util && fixed broken tests #6187

debris commented Jul 29, 2017

debris commented Jul 30, 2017

eira-fransham left a comment

eira-fransham Aug 1, 2017

debris Aug 1, 2017

eira-fransham Aug 1, 2017

debris Aug 1, 2017

eira-fransham Aug 1, 2017

eira-fransham Aug 1, 2017

debris Aug 1, 2017

eira-fransham Aug 1, 2017

eira-fransham Aug 1, 2017

debris Aug 1, 2017

eira-fransham Aug 1, 2017

debris Aug 1, 2017

eira-fransham Aug 1, 2017

debris Aug 1, 2017

eira-fransham Aug 1, 2017

eira-fransham Aug 1, 2017

debris Aug 1, 2017

eira-fransham Aug 1, 2017

eira-fransham Aug 1, 2017

debris Aug 1, 2017

debris Aug 1, 2017

eira-fransham commented Aug 1, 2017

eira-fransham left a comment •

edited

Loading

debris commented Aug 1, 2017

removed std reexports from util && fixed broken tests #6187

removed std reexports from util && fixed broken tests #6187

Conversation

debris commented Jul 29, 2017

debris commented Jul 30, 2017

eira-fransham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham commented Aug 1, 2017

eira-fransham left a comment • edited Loading

Choose a reason for hiding this comment

debris commented Aug 1, 2017

eira-fransham left a comment •

edited

Loading