Skip to content

Conversation

marioevz
Copy link
Member

This PR includes a refactoring of the Test Verification syntax to make the test cases much more readable and explicit on what they expect as outcome.

The Expect* methods are introduced to allow test cases to explicitly indicate what to expect from each call they perform, to either the Engine API or the Eth JSON-RPC.

E.g. this verification:

fcResp, err := t.Engine.EngineForkchoiceUpdatedV1(t.Engine.Ctx(), &forkchoiceState, nil)
if err != nil {
	t.Fatalf("FAIL (%s): ForkchoiceUpdated under PoW rule returned error (Expected INVALID_TERMINAL_BLOCK): %v, %v", t.TestName, err)
}
if fcResp.PayloadStatus.Status != "INVALID_TERMINAL_BLOCK" {
	t.Fatalf("INFO (%v): Incorrect EngineForkchoiceUpdatedV1 response for invalid PoW parent (Expected PayloadStatus.Status=INVALID_TERMINAL_BLOCK): %v", t.TestName, fcResp.PayloadStatus.Status)
}
if fcResp.PayloadStatus.LatestValidHash != nil {
	t.Fatalf("INFO (%v): Incorrect EngineForkchoiceUpdatedV1 response for invalid PoW parent (Expected PayloadStatus.LatestValidHash==nil): %v", t.TestName, fcResp.PayloadStatus)
}

becomes:

t.TestEngine.
	TestEngineForkchoiceUpdatedV1(&forkchoiceState, nil).
	ExpectPayloadStatus(InvalidTerminalBlock).
	ExpectLatestValidHash(nil)

The verification fail check is now embedded within all the Expect* methods and, unless they explicitly expect an error (e.g. ExpectError), the call automatically checks for err == nil.

@fjl
Copy link
Collaborator

fjl commented Apr 17, 2022

The method chaining pattern is cute, but not typically used in Go code. It is preferable to assign the receiver to a single letter variable and then call the methods on that, e.g.

r := t.TestEngine.TestEngineForkchoiceUpdatedV1(&forkchoiceState, nil)
r.ExpectPayloadStatus(InvalidTerminalBlock)
r.ExpectLatestValidHash(nil)

The reasoning behind this is:

  • There are usually none (or very few) LoC saved by chaining.
  • It can be more difficult to insert/remove checks if chaining is used. For example, when removing the last chained call, the . on the previous line must be removed also.

@fjl
Copy link
Collaborator

fjl commented Apr 17, 2022

Note, this means Expect* should have no return value.

@marioevz marioevz force-pushed the test-syntax-refactor branch from 661282f to 3c6ea68 Compare April 18, 2022 18:46
@fjl
Copy link
Collaborator

fjl commented Apr 19, 2022

Thanks, it looks a bit better with your changes.

@fjl fjl merged commit 1a727dd into ethereum:master Apr 22, 2022
racytech pushed a commit to racytech/hive that referenced this pull request Apr 4, 2025
racytech pushed a commit to racytech/hive that referenced this pull request Apr 4, 2025
🤖 I have created a release *beep* *boop*
---


##
[2.1.0](ethpandaops/ethereum-package@2.0.0...2.1.0)
(2024-03-28)


### Features

* add beacon snooper
([ethereum#520](ethpandaops/ethereum-package#520))
([7e36191](ethpandaops/ethereum-package@7e36191))
* add BN<>CL compatibility matrix to readme
([ethereum#519](ethpandaops/ethereum-package#519))
([177beeb](ethpandaops/ethereum-package@177beeb))
* add grandine
([ethereum#517](ethpandaops/ethereum-package#517))
([3ac4d2a](ethpandaops/ethereum-package@3ac4d2a))
* enable preset to be set, mainnet/minimal
([ethereum#524](ethpandaops/ethereum-package#524))
([f6e1b13](ethpandaops/ethereum-package@f6e1b13))
* make deneb genesis default
([ethereum#518](ethpandaops/ethereum-package#518))
([49509b9](ethpandaops/ethereum-package@49509b9))
* make keymanager optional
([ethereum#523](ethpandaops/ethereum-package#523))
([969012c](ethpandaops/ethereum-package@969012c))
* update verkle genesis + add besu support to verkle testing
([ethereum#512](ethpandaops/ethereum-package#512))
([0615cd1](ethpandaops/ethereum-package@0615cd1))


### Bug Fixes

* architecture.md
([ethereum#514](ethpandaops/ethereum-package#514))
([f0ec4f0](ethpandaops/ethereum-package@f0ec4f0))
* blobscan network name
([ethereum#516](ethpandaops/ethereum-package#516))
([83c2a55](ethpandaops/ethereum-package@83c2a55))
* **blobscan:** update healthcheck endpoint
([ethereum#513](ethpandaops/ethereum-package#513))
([8b2fc61](ethpandaops/ethereum-package@8b2fc61))
* separate vc
([ethereum#526](ethpandaops/ethereum-package#526))
([baa04e9](ethpandaops/ethereum-package@baa04e9))
* Updated Readme with VCs supported by Grandine BN
([ethereum#527](ethpandaops/ethereum-package#527))
([9cbe0b3](ethpandaops/ethereum-package@9cbe0b3))
* use correct dora & assertoor images
([ethereum#522](ethpandaops/ethereum-package#522))
([2a8d73a](ethpandaops/ethereum-package@2a8d73a))
* use new validator names in assertoor config
([ethereum#521](ethpandaops/ethereum-package#521))
([f595eb9](ethpandaops/ethereum-package@f595eb9))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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