[spike] Refactor TestIssueAtomicTxs as an integration test #377
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marun
force-pushed
the
vm-integration-fixture
branch
from
b001053
to
ab74f52
Compare
marun
force-pushed
the
vm-integration-fixture
branch
from
58dd28a
to
b384047
Compare
marun
commented
Nov 8, 2023
Merged
1 task
marun
force-pushed
the
vm-integration-fixture
branch
2 times, most recently
from
c1a8164
to
8746622
Compare
marun
added
testing
marun
force-pushed
the
vm-integration-fixture
branch
3 times, most recently
from
308266a
to
923dc81
Compare
marun
force-pushed
the
vm-integration-fixture
branch
2 times, most recently
from
4fe7024
to
4843b22
Compare
marun
requested review from
aaronbuchwald,
darioush and
ceyonur
as code owners
marun
force-pushed
the
vm-integration-fixture
branch
3 times, most recently
from
420e05a
to
8860ccd
Compare
ceyonur
reviewed
Dec 4, 2023
marun
changed the title
marun
force-pushed
the
vm-integration-fixture
branch
from
e1b330b
to
e9ba3d7
Compare
darioush
reviewed
Mar 18, 2024
| assert.Equal(t, uint64(2), height, "expected height of indexed export tx to be 2") | ||
| assert.Equal(t, indexedExportTx.ID(), exportTx.ID(), "expected ID of indexed import tx to match original txID") | ||
| } | ||
|
|
||
| func TestBuildEthTxBlock(t *testing.T) { |
There was a problem hiding this comment.
I think we should be confident that the fixture can interface with most if not all the tests in vm_test.go vm_warp_test.go, (eg, do some of these instantiate more than 1 vm?)
Some of the tests in syncervm_test.go are also candidates, but somewhat a stretch goal.
| var fixture IntegrationFixture | ||
| if e2e.Env == nil { | ||
| // Shared network fixture not initialized, return a vm fixture | ||
| fixture = evm.CreateVMFixture(ginkgo.GinkgoT()) |
There was a problem hiding this comment.
smaller comment: Should we consider calling the light (VM) fixture from the unit tests? This would continue running them with race detection, and on the different OS-es.
|
I like what this change achieves. I think it makes sense to run the "VM-level unit tests" against a full node. |
darioush
pushed a commit
that referenced
this pull request
Apr 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why this should be merged
This refactor is intended to demonstrate the potential of writing integration tests against a fixture interface that can be implemented by both a bare VM and a network API. This should support more comprehensive test coverage for minimal additional cost.
How this works
How this was tested
A new integration test suite that includes the refactored test is executed in CI (via
scripts/tests.integration.sh) as part of the unit test job (with vm fixture) and a new integration test job (with network fixture).TODO