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

Dynamic Chain Test Language Package With YAML Parser #681

Merged
merged 46 commits into from
Nov 7, 2018

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented Oct 18, 2018

Resolves #420


Description

This PR incorporates the YAML format for fork-choice and chain tests as outlined in the Ethereum 2.0 spec and the markdown file here by @djrtwo. This PR does not yet implement the simulated backend that will act as the runner for the tests but simply includes a parser for supplied yaml files to extract test case parameters.

Requirements for Merge

  • Basic Refactoring to Allow for Simulated Backend
  • Basic Simulated Backend Definition
  • Refactor Config to Allow for Custom Parameters for Simulated Runs
  • YAML Parser From File
  • Include a Basic Chaintest File
  • README on Using the Chaintest Runner

Important

This PR does not actually run the full tests but instead just sets up the necessary framework and testing format required to do so. The test implementation will be part of #718.

@rauljordan rauljordan added this to the Ruby - v0.1 milestone Oct 18, 2018
@rauljordan rauljordan self-assigned this Oct 18, 2018
@rauljordan rauljordan changed the title Dynamic Chain Test Package With YAML Parser Dynamic Chain Test Language Package With YAML Parser Oct 18, 2018
@codecov
Copy link

codecov bot commented Oct 20, 2018

Codecov Report

Merging #681 into master will decrease coverage by 0.05%.
The diff coverage is 41.86%.

@@            Coverage Diff             @@
##           master     #681      +/-   ##
==========================================
- Coverage    75.2%   75.14%   -0.06%     
==========================================
  Files          60       59       -1     
  Lines        3815     3834      +19     
==========================================
+ Hits         2869     2881      +12     
- Misses        684      692       +8     
+ Partials      262      261       -1

@@ -232,13 +232,13 @@ func TestValidatorsBySlotShardSmallValidatorSet(t *testing.T) {
for i := 0; i < len(shardAndCommitteeArray); i++ {
shardAndCommittees := shardAndCommitteeArray[i].ArrayShardAndCommittee
if len(shardAndCommittees) != 1 {
t.Fatalf("Expected %d committee per slot: got %d", params.GetConfig().MinCommiteeSize, 1)
t.Fatalf("Expected %d committee per slot: got %d", params.GetConfig().MinCommitteeSize, 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were a few typos with Committee that needed to be fixed

.codecov.yml Outdated
project:
default:
target: auto
threshold: 1%
Copy link
Member

Choose a reason for hiding this comment

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

Can we include this in a different PR?

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, it looks like its already in master!

"test_format.go",
],
importpath = "github.com/prysmaticlabs/prysm/beacon-chain/chaintest/backend",
visibility = ["//beacon-chain:__subpackages__"],
Copy link
Member

Choose a reason for hiding this comment

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

Please limit to //beacon-chain/chaintest:__subpackages__


// setupDB instantiates and returns a simulated backend BeaconDB instance.
func setupDB() (*db.BeaconDB, error) {
path := path.Join(os.TempDir(), "/simulateddb")
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause issues if you are running tests in parallel?
Maybe a random named directory would be ideal.

@@ -0,0 +1,82 @@
// Package backend contains utilities for simulating an entire
// ETH 2.0 beacon chain in-memory for e2e tests and benchmarking
Copy link
Member

Choose a reason for hiding this comment

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

Is this true? The database writes to disk so its not fully in-memory. Was that the intention?

RandaoCommitment: randaoReveal[:],
}
}
// TODO(#718): Next step is to update and save the blocks specified
Copy link
Member

Choose a reason for hiding this comment

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

Please add to your PR description that this PR does not actually run the tests and that the test implementation is part of #718

@rauljordan
Copy link
Contributor Author

PTAL @prestonvanloon

@@ -25,7 +27,7 @@ func teardownDB(db *db.BeaconDB) {
if err := db.Close(); err != nil {
log.Fatalf("failed to close database: %v", err)
}
path := path.Join(os.TempDir(), "/simulateddb")
path := path.Join(os.TempDir(), fmt.Sprintf("/%d", rand.Int()))
Copy link
Member

Choose a reason for hiding this comment

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

This won't clean up the same random number will it?

I think you need to keep track of this number somehow, maybe this path could be db.DatabasePath

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

Yeah agreed, maybe try to keep track of the number of simulated backend instances running and assign your path that way.

"github.com/prysmaticlabs/prysm/beacon-chain/db"
)

// setupDB instantiates and returns a simulated backend BeaconDB instance.
func setupDB() (*db.BeaconDB, error) {
path := path.Join(os.TempDir(), "/simulateddb")
path := path.Join(os.TempDir(), fmt.Sprintf("/%d", rand.Int()))
Copy link
Member

Choose a reason for hiding this comment

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

Please prefix with something human readable: prysmsimulateddb-%d or similar

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -25,7 +27,7 @@ func teardownDB(db *db.BeaconDB) {
if err := db.Close(); err != nil {
log.Fatalf("failed to close database: %v", err)
}
path := path.Join(os.TempDir(), "/simulateddb")
path := path.Join(os.TempDir(), fmt.Sprintf("/%d", rand.Int()))
Copy link
Member

Choose a reason for hiding this comment

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

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High High priority item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants