Skip to content
This repository has been archived by the owner on Jun 21, 2020. It is now read-only.

FIX: Fixed an issue with the test_write_epoch_state unit test. #118

Closed
wants to merge 9 commits into from

Conversation

fredfortier
Copy link
Contributor

@fredfortier fredfortier commented Apr 11, 2019

This test runs before warming up the storage dirs. The fix is temporary. I'm currently working on improving the unit test organization. I will submit a PR that fixes this permanently and handles setup more gracefully. In the meantime, this will fix the test. I did not detect this locally because the test setup did not delete the local storage, which I will remediate in the next PR. I do not know why the CI behaved differently on this branch and develop.

… test runs before warming up the storage dirs. The fix is temporary. I'm currently working on improving the unit test organization. I will create a PR with a more elegant permanent fix. I did not detect this locally because the storage dirs existed. I will account for this in the future.
…hereum-types` broke some things in our code.
@fredfortier
Copy link
Contributor Author

fredfortier commented Apr 11, 2019

The CI tests were failing after commit: 45facac.

error: Could not compile `enigma-tools-u`.
error[E0599]: no method named `to_hex` found for type `web3::types::H160` in the current scope
   --> src/web3_utils/w3utils.rs:219:32
    |
219 |         let deployer = account.to_hex();
    |                                ^^^^^^

After investigation, I determined that this was the culprit: tomusdrw/rust-web3@91639ff. Specifically, the problem is related to the ethereum-types upgrade. This broke the CI tests in the develop branch as well.

For now, I resolved the issue by specifying the last good revision of rust-web3 in the Cargo.toml files that reference it. I will try to adjust to the latest revision in a separate branch after finalizing my other tasks.

@elichai
Copy link
Contributor

elichai commented Apr 15, 2019

Yeah in general it was a bad idea that we kept using his git master as a dependency.
Can we use a published version maybe?

@elichai
Copy link
Contributor

elichai commented Apr 15, 2019

And I'm not sure what "warming up" means.

@fredfortier
Copy link
Contributor Author

I'm not sure of the reason why we kept using the master. I assume that some of our PRs were merged but never published. I'll trying using a publish version. By warming up, I refer to the setup of test prerequisites.

@elichai
Copy link
Contributor

elichai commented Apr 15, 2019

Still don't really understand what setup is this.
I can try running the tests in develop myself and see if/why they fail but I'm out of the office for now(sick)

@fredfortier
Copy link
Contributor Author

fredfortier commented Apr 15, 2019

The setup required for this particular test is to create the .enigma folder structure. You can see how I addressed it in the commit. Take you time. I hope that you feel better!

@elichai
Copy link
Contributor

elichai commented Apr 16, 2019

@fredfortier I tested and #120 pass all the tests. what am I missing?
what test is failing because of this "warmup" issues?

@elichai
Copy link
Contributor

elichai commented Apr 16, 2019

So If I understood correctly, the "warm up" problem is just that your test assumes ~/.enigma exists and tries to write a file into it, am I correct?.
if so the solution is to create that dir if missing using fs::create_dir() or fs::create_dir_all() before trying to open a file there...

@elichai
Copy link
Contributor

elichai commented Apr 17, 2019

Please do this in a new PR without the changes to web3 and rustc-demangle

@fredfortier
Copy link
Contributor Author

I'll just included in in PR #121

@AvishaiW AvishaiW deleted the fix-epoch-state-serialization branch December 22, 2019 14:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants