Conversation
…ithub tests could use spec code
|
Everythng is passed. On hold until generated tests repository is online. |
# Conflicts: # consensus/src/main/java/org/ethereum/beacon/consensus/spec/HelperFunction.java # test/src/test/java/org/ethereum/beacon/test/ShuffleTests.java
|
There are a lot of things broken after merging v0.6.1:
Everything else is passed. |
mkalinin
left a comment
mkalinin
left a comment
There was a problem hiding this comment.
Great job, in general! 👍
Requested only a few changes.
# Conflicts: # consensus/src/main/java/org/ethereum/beacon/consensus/spec/HelperFunction.java # test/src/test/java/org/ethereum/beacon/test/ShuffleTests.java
…s in operations state tests
|
@mkalinin fixed everything according to your review |
mkalinin
left a comment
mkalinin
left a comment
There was a problem hiding this comment.
Found a couple of new things that might worth a fix. In spite of this, I think it's ready to get merged!
| } catch (Exception e) { | ||
| return VerificationResult.failedResult(e.getMessage()); | ||
| } catch (SpecCommons.SpecAssertionFailed e) { | ||
| String error = e.getStackTrace().length > 0 ? e.getStackTrace()[1].toString() : "SpecAssertion"; |
There was a problem hiding this comment.
Literally, e.getStackTrace().length > 0 check doesn't guarantee that e.getStackTrace()[1] won't turn with index out of bounds; but I believe it's OK for this particular case.
Do you mind overriding getMessage for SpecAssertionFailed which would execute this line of code and as a result return error string?
| DepositVerifier depositVerifier = new DepositVerifier(spec); | ||
| assert depositVerifier.verify(deposit, state).isPassed(); | ||
| spec.process_deposit((MutableBeaconState) state, deposit); | ||
| } catch (SpecCommons.SpecAssertionFailed | AssertionError ex) { |
There was a problem hiding this comment.
We don't use assert in the spec, do we? I am thinking of some rare cases when AssertionError has been due to odd state of some object (usual way assert is made for); in that case I'd rather prefer to see this exception than get it swallowed here.
|
Update. Currently failed:
All tests are blocked by resolving of this issue: ethereum/consensus-spec-tests#1 |
…are updated to the latest spec
… algo used in fixtures generation
This reverts commit 3eb2c30
zilm13
changed the title
2 participants
Restored
Moving to new tests from https://github.com/ethereum/eth2.0-specs/tree/dev/test_generators
Everything done and passed except 2 operations/attestations tests.
Keep
WIPuntil we have repository with automatically builtyamltests, plusgradletest task should be updated to match this changes. Currently it's done manually.