test: treat libevm errors as test failures#115
Conversation
JonathanOppenheimer
requested review from
ARR4N and
StephenButtolph
as code owners
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
| # Test binaries | ||
| *.test |
There was a problem hiding this comment.
I generated a bunch of sae.test etc. I don't want to commit them and assume we don't want them here.
There was a problem hiding this comment.
How did you manage to do that? Something's weird and this shouldn't be necessary.
There was a problem hiding this comment.
Just go test -c
There was a problem hiding this comment.
I can remove this from the .gitignore if you want
| # Test binaries | ||
| *.test |
There was a problem hiding this comment.
How did you manage to do that? Something's weird and this shouldn't be necessary.
JonathanOppenheimer
force-pushed
the
JonathanOppenheimer/enhance-libevm-logging
branch
from
53bc942 to
82f0ef0
Compare
|
Lets change this to error the test if an error happens rather than a warn, as geth doesn't seem to care too much about warns. |
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
|
|
||
| mempoolConf := legacypool.DefaultConfig // copies | ||
| mempoolConf.Journal = "/dev/null" | ||
| mempoolConf.Journal = filepath.Join(tb.TempDir(), "transactions.rlp") |
There was a problem hiding this comment.
Was there a test failure after this? It didn't write to the journal?
There was a problem hiding this comment.
I don't think this is actually necessary and a vestige of prior iterations of this PR. removed.
There was a problem hiding this comment.
actually this seems to be semi necessary
There was a problem hiding this comment.
For some reason sae/TestWorstCase fails without this.
Co-authored-by: Austin Larson <78000745+alarso16@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
Co-authored-by: Austin Larson <78000745+alarso16@users.noreply.github.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
powerslider
left a comment
powerslider
left a comment
There was a problem hiding this comment.
Overall LGTM! Added a slight simplification for the libevm logger.
thanks tsvetan! |
Co-authored-by: Tsvetan Dimitrov <tsvetan.dimitrov23@gmail.com> Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
JonathanOppenheimer
force-pushed
the
JonathanOppenheimer/enhance-libevm-logging
branch
from
910457e to
d94ee19
Compare
@powerslider -- it doesn't appear your suggestion works - it still races. I'll revert the past 3 commits unless you object. |
- Guard `log.SetDefault` with an `atomic.Bool` so only the first call takes effect. This removes the need for the `withoutGlobalLogger` option machinery in `newSUT` while preventing data races from parallel fuzz sub-tests each calling `EnableLibEVMTBLogger` via `newSUT`. - `FuzzOpCodes` now calls `EnableLibEVMTerminalLogger(f)` before `f.Fuzz()` since Go forbids `testing.F.Logf` inside fuzz targets. Sub-tests' calls to `EnableLibEVMTBLogger` are no-ops. Signed-off-by: Tsvetan Dimitrov (tsvetan.dimitrov@avalabs.org) --------- Signed-off-by: Tsvetan Dimitrov (tsvetan.dimitrov23@gmail.com)
Signed-off-by: Jonathan Oppenheimer <147infiniti@gmail.com>
5 participants
Closes ava-labs/avalanchego#5271!
For context, we decided that libevm warnings should be treated as warnings. I added the new logger in the suggested places - let me know if we think it would be useful in any additional places!
This PR:
EnableLibEVMTBLoggerto redirect libevm/geth logs totesting.TBduring tests, failing on errorssae,saexec,txgossip) so libevm errors are never silently swallowedAdditionally, this PR should not be merged until ava-labs/libevm#269 is merged.