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

E2E refactoring: bootnode #8659

Merged
merged 11 commits into from
Mar 24, 2021
Merged

E2E refactoring: bootnode #8659

merged 11 commits into from
Mar 24, 2021

Conversation

farazdagi
Copy link
Contributor

@farazdagi farazdagi commented Mar 24, 2021

What type of PR is this?

Other / Refactoring

What does this PR do? Why is it needed?

  • This is the first of the series of the PRs that will gradually update E2E tests. Refactoring is complete in E2E refactoring #8643, however, that PR is too big, for easy and proper review. Hence, as suggested by @rkapka slicing it into parts.
  • In this PR:
    • ComponentRunner interface is introduced: all components will have this interface, so within test spawning and waiting for component to fully start will be easier.
    • Adds logger to components package (we will not rely on t *testing.T in components for logging)
    • StartBootnode() is moved into separate boot_node.go file (originally we used beacon-node as a bootnode, but now have dedicated component, so it is abstracted in a separate file for consistency)
    • Within boot_node.go a new type is introduced, namely BootNode (it embeds ComponentRunner). Basically, this component has the very same code from StartBootnode() function, but abstracted within component interface. One major difference, to start the process exec.CommandContext() is used instead of exec.Command() -- so, we can terminate running process at the end of the test.
    • Within scope of this PR, introduced component is not used within endtoend_test.go:runEndToEndTest() -- this will be enabled in the last PR, which will update endtoend_test.go heavily to rely on errgroup and revamped components. Within that PR, component will be run as following (for details see E2E refactoring #8643 where BootNode is fully integrated):
ctx, done := context.WithCancel(context.Background())
g, ctx := errgroup.WithContext(ctx)

// Boot node.
bootNode := components.NewBootNode(config)
g.Go(func() error {
	return bootNode.Start(ctx)
})

// Run E2E evaluators and tests.
g.Go(func() error {
	// When everything is done, cancel parent context (will stop all spawned nodes).
	defer func() {
		log.Info("All E2E evaluations are finished, cleaning up")
		done()
	}()
        // Here is code for running evaluators, at the end of this func, close the parent context.
})

if err := g.Wait(); err != nil && !errors.Is(err, context.Canceled) {
	// At the end of the main evaluator goroutine all nodes are killed, no need to fail the test.
	if strings.Contains(err.Error(), "signal: killed") {
		return
	}
	t.Fatalf("E2E test ended in error: %v", err)
}

All nodes that depend on BootNode will be able to wait on its started channel (here is example of waiting for ETH1 node):

// ETH1 node.
eth1Node := components.NewEth1Node(config)
g.Go(func() error {
	return eth1Node.Start(ctx)
})
// Send partial deposits.
g.Go(func() error {
	if err := helpers.ComponentsStarted(ctx, []e2etypes.ComponentRunner{eth1Node}); err != nil {
		return fmt.Errorf("sending and mining deposits require ETH1 node to run: %w", err)
	}
	return components.SendAndMineDeposits(eth1Node.KeystorePath(), minGenesisActiveCount, 0, true /* partial */)
})

Which issues(s) does this PR fix?

N/A

Other notes for review

  • Extracted from E2E refactoring #8643
  • ❗❗ This PR doesn't amend code in any current execution path, that's it only introduces new types/methods, but doesn't use them. Therefore, it should be safe to merge it -- but still, please, do review carefully.

@farazdagi farazdagi self-assigned this Mar 24, 2021
@farazdagi farazdagi mentioned this pull request Mar 24, 2021
6 tasks
@farazdagi farazdagi marked this pull request as ready for review March 24, 2021 15:02
@farazdagi farazdagi requested a review from a team as a code owner March 24, 2021 15:02
@prylabs-bulldozer prylabs-bulldozer bot merged commit 89da5d1 into develop Mar 24, 2021
@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #8659 (667f7e2) into develop (eca67ce) will decrease coverage by 0.01%.
The diff coverage is 67.85%.

@@             Coverage Diff             @@
##           develop    #8659      +/-   ##
===========================================
- Coverage    61.21%   61.20%   -0.02%     
===========================================
  Files          493      494       +1     
  Lines        34296    34343      +47     
===========================================
+ Hits         20996    21021      +25     
- Misses       10169    10186      +17     
- Partials      3131     3136       +5     

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

Successfully merging this pull request may close these issues.

2 participants