feat(forge test): stream test results #798

lattejed · 2022-02-23T11:44:39Z

@gakonst this adds streaming for test results. Makes the UI more responsive for large projects / older machines.

cli/src/cmd/test.rs

forge/src/multi_runner.rs

cli/src/cmd/test.rs

…f a closure

onbjerg

Tested with forge test, forge test --gas-report and forge test -vvvvv and it all worked as expected (tested on solmate). It didn't make much of a difference on my machine, but I'm sure it will on someone elses. Great work!

onbjerg

Nvm, it actually isn't really using the channel as we discussed - the call to runner.test blocks since we're waiting for results, so the receiving/outputting loop is not run until that finishes and the behavior is the same as if we were not using a channel

lattejed · 2022-02-24T17:32:04Z

@onbjerg yeah, I think we'll have to ditch the mpsc since it will block if we put runner.test after it.

gakonst · 2022-02-24T17:34:34Z

Also something to consider: AFAIK this streams results on a per-contract basis, do you think we could do something like a streaming terminal that fills stdout across the entire board, by dynamically adding new contract tags as tests for that contract complete?

example:

$ forge test
< no output>

then when Contract::testContract1() completes it does:

Contract.sol:Contract
testContract1()

then when another contract test completes, it'd do

Contract.sol:Contract
testContract1()

Contract2.sol:Contract2
testContract2()

and then when contract 1's other function completes it'd show

Contract.sol:Contract
testContract1()
testContract1Other()

Contract2.sol:Contract2
testContract2()

lattejed · 2022-02-24T17:48:46Z

@onbjerg can keep the mpsc by sticking the while loop in a separate thread.

@gakonst could store the lines in memory and redraw the whole set as it updates. I'm not sure what terminal lib would work but seems doable?

I'd worry about messing up scrolling or having updates appear offscreen though. If it does behave with scrolling though it would be pretty cool.

lattejed · 2022-02-24T18:20:08Z

@onbjerg if you're happy with this please go ahead with it. I'll have to give what @gakonst is suggesting some thought and I'll open a separate PR if it seems doable.

onbjerg · 2022-02-24T19:43:07Z

I think this is good for terminals where we cannot move the cursor back (like CIs) and then for interactive terminals we could do what @gakonst suggested with https://github.com/fdehau/tui-rs (see the paragraph example). Will give this a go again

onbjerg

Works as intended now 😄

lattejed · 2022-02-25T04:12:45Z

This does appear to be running tests in sync again whereas an earlier version did not. Needs more investigation

lattejed · 2022-02-25T06:26:18Z

Actually, no, this seems to be working as expected. These are tests with hashing in a loop, the last part of the test fn name is how many times the loop runs.

First test setup:

forge test
Compiling...
No files changed, compilation skipped

Running 1 test for CTest.json:CTest
[PASS] test_keccak256_16384() (gas: 6162268)

Running 1 test for ATest.json:ATest
[PASS] test_keccak256_16384() (gas: 6162268)

Running 1 test for BTest.json:BTest
[PASS] test_keccak256_16384() (gas: 6162268)

forge test
Compiling...
No files changed, compilation skipped

Running 1 test for ATest.json:ATest
[PASS] test_keccak256_16384() (gas: 6162268)

Running 1 test for BTest.json:BTest
[PASS] test_keccak256_16384() (gas: 6162268)

Running 1 test for CTest.json:CTest
[PASS] test_keccak256_16384() (gas: 6162268)

forge test
Compiling...
No files changed, compilation skipped

Running 1 test for ATest.json:ATest
[PASS] test_keccak256_16384() (gas: 6162268)

Running 1 test for CTest.json:CTest
[PASS] test_keccak256_16384() (gas: 6162268)

Running 1 test for BTest.json:BTest
[PASS] test_keccak256_16384() (gas: 6162268)

Tests print at approximately the same time but in indeterminate order. This is expected.

Second test setup:

forge test
Compiling...
Compiling 2 files with 0.8.12
Compilation finished successfully
Success

Running 1 test for CTest.json:CTest
[PASS] test_keccak256_1() (gas: 1492)

Running 1 test for BTest.json:BTest
[PASS] test_keccak256_256() (gas: 65232)

Running 1 test for ATest.json:ATest
[PASS] test_keccak256_16384() (gas: 6162268)

forge test
Compiling...
No files changed, compilation skipped

Running 1 test for CTest.json:CTest
[PASS] test_keccak256_1() (gas: 1492)

Running 1 test for BTest.json:BTest
[PASS] test_keccak256_256() (gas: 65232)

Running 1 test for ATest.json:ATest
[PASS] test_keccak256_16384() (gas: 6162268)

Tests stream in that order consistently. This is also expected.

@gakonst @onbjerg we agree that's expected behavior, right? Maybe I'm overlooking something.

gakonst

Merging cautiously. This PR made me think that it's worth extracting our integration tests from here to Criterion benchmarks

gakonst · 2022-02-25T21:02:47Z

cli/src/cmd/test.rs

-            }
+        let mut gas_report = GasReport::new(gas_reports.1);
+        let (tx, rx) = channel::<(String, BTreeMap<String, TestResult>)>();
+        let known_contracts = runner.known_contracts.clone();


Is this not expensive to clone each time?

lattejed · 2022-02-26T07:20:01Z

My intuition would be no but I'd be happy to quantify it somehow -- or possibly just remove the clone() -- if you'd like

gakonst · 2022-02-26T07:27:22Z

Let's leave as-is, no big deal and want to be respectful of your time invested in this already. Thank you again!

lattejed · 2022-02-26T07:43:07Z

Thanks @gakonst 🤝

lattejed added 4 commits February 23, 2022 18:13

Added streaming to tests stdout, misc capitalization fixes

4976461

Formatting

0245816

Fix clippy warnings

a817670

Added doc to mutex

cde74d3

onbjerg mentioned this pull request Feb 23, 2022

Re-implement new features for REVM #799

Closed

8 tasks

Made compilation messages consistent and fixed broken tests

42a823b

onbjerg reviewed Feb 23, 2022

View reviewed changes

cli/src/cmd/test.rs Show resolved Hide resolved

forge/src/multi_runner.rs Show resolved Hide resolved

onbjerg reviewed Feb 23, 2022

View reviewed changes

forge/src/multi_runner.rs Outdated Show resolved Hide resolved

onbjerg added the T-feature Type: feature label Feb 23, 2022

lattejed added 4 commits February 23, 2022 21:31

Combine test and test_stream

1aa1061

Fix broken tests

84b6b87

Merge branch 'master' into lattejed/stream_tests

a9c3c93

Fix more broken tests

ae918ef

onbjerg reviewed Feb 24, 2022

View reviewed changes

cli/src/cmd/test.rs Outdated Show resolved Hide resolved

Refactored MultiContractRunner::test to accept mpsc::Sender instead o…

ab5492f

…f a closure

onbjerg approved these changes Feb 24, 2022

View reviewed changes

onbjerg requested changes Feb 24, 2022

View reviewed changes

Fix unintentional blocking in streaming of test results

d4ead3f

onbjerg approved these changes Feb 24, 2022

View reviewed changes

gakonst approved these changes Feb 26, 2022

View reviewed changes

gakonst merged commit 135cb12 into foundry-rs:master Feb 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(forge test): stream test results #798

feat(forge test): stream test results #798

lattejed commented Feb 23, 2022

onbjerg left a comment

onbjerg left a comment

lattejed commented Feb 24, 2022

gakonst commented Feb 24, 2022 •

edited

Loading

lattejed commented Feb 24, 2022 •

edited

Loading

lattejed commented Feb 24, 2022

onbjerg commented Feb 24, 2022 •

edited

Loading

onbjerg left a comment

lattejed commented Feb 25, 2022

lattejed commented Feb 25, 2022 •

edited

Loading

gakonst left a comment

gakonst Feb 25, 2022

lattejed commented Feb 26, 2022

gakonst commented Feb 26, 2022

lattejed commented Feb 26, 2022

feat(forge test): stream test results #798

feat(forge test): stream test results #798

Conversation

lattejed commented Feb 23, 2022

onbjerg left a comment

Choose a reason for hiding this comment

onbjerg left a comment

Choose a reason for hiding this comment

lattejed commented Feb 24, 2022

gakonst commented Feb 24, 2022 • edited Loading

lattejed commented Feb 24, 2022 • edited Loading

lattejed commented Feb 24, 2022

onbjerg commented Feb 24, 2022 • edited Loading

onbjerg left a comment

Choose a reason for hiding this comment

lattejed commented Feb 25, 2022

lattejed commented Feb 25, 2022 • edited Loading

gakonst left a comment

Choose a reason for hiding this comment

gakonst Feb 25, 2022

Choose a reason for hiding this comment

lattejed commented Feb 26, 2022

gakonst commented Feb 26, 2022

lattejed commented Feb 26, 2022

gakonst commented Feb 24, 2022 •

edited

Loading

lattejed commented Feb 24, 2022 •

edited

Loading

onbjerg commented Feb 24, 2022 •

edited

Loading

lattejed commented Feb 25, 2022 •

edited

Loading