Skip to content

Commit

Permalink
Fix flaky test (#2951)
Browse files Browse the repository at this point in the history
# Description
Another flaky test caused CI to
[fail](https://github.com/cowprotocol/services/actions/runs/10723467528/job/29736827126?pr=2950)
on an unrelated change (also worked locally).
The test in question fails because no trade happens. After comparing the
logs of a failed CI run with a successful local run it looks like the
order in question never makes it into the auction because 1 of the
native prices is always missing.
The test actually sets up all the liquidity that is needed but in the
failed run the solver was never able to compute a solution.
The driver correctly found the existing pool it was supposed to use but
apparently the driver's view of the pool was not up-to-date.
As an optimization the driver is allowed to use stale liquidity data for
quotes (but not for solving auctions). This was already the cause of
flaky tests in the past and the fix was to keep mining new blocks until
the driver flushes out the cached liquidity forcing it to see the latest
state of the chain which then allows it to finally compute a native
price.

To make the whole thing work reliably in CI I also had to use a tokio
runtime with 2 worker threads.

# Changes
* mine a bunch of blocks to flush out stale cached liquidity
* also changed how we wait for the trade to happen since synchronizing
on the size of the auction is prone to race * conditions
* To test the change sufficiently I added a new opt-in flaky test runner
that runs 1 test in a loop to see if it works.
Also mentioned that in the readme.


## How to test
Used new flaky test runner to run the test 500 times
  • Loading branch information
MartinquaXD authored Sep 6, 2024
1 parent 990ce6c commit 6c73eb6
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 34 deletions.
37 changes: 37 additions & 0 deletions .github/workflows/pull-request.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -180,3 +180,40 @@ jobs:
- run: node_modules/.bin/swagger-cli validate crates/orderbook/openapi.yml
- run: node_modules/.bin/swagger-cli validate crates/driver/openapi.yml
- run: node_modules/.bin/swagger-cli validate crates/solvers/openapi.yml

run-flaky-test:
# to debug a flaky test set `if` to true and configure the flaky test in the `run` step
if: ${{ false }}
timeout-minutes: 60
runs-on: ubuntu-latest
env:
# Shrink artifact size by not including debug info. Makes build faster and shrinks cache.
CARGO_PROFILE_DEV_DEBUG: 0
CARGO_PROFILE_TEST_DEBUG: 0
CARGO_TERM_COLOR: always
TOML_TRACE_ERROR: 1
steps:
- uses: actions/checkout@v3
with:
ref: ${{ github.event.pull_request.head.sha }}
- run: rustup toolchain install stable --profile minimal
- uses: foundry-rs/foundry-toolchain@v1
- uses: Swatinem/rust-cache@v2
# Start the build process in the background. The following cargo test command will automatically
# wait for the build process to be done before proceeding.
- run: cargo build -p e2e --tests &
- uses: taiki-e/install-action@nextest
- uses: yu-ichiro/spin-up-docker-compose-action@v1
with:
file: docker-compose.yaml
up-opts: -d db migrations
- name: Run test in a loop
run: |
attempt=1
while true; do
echo "Running test attempt #$attempt"
if ! cargo nextest run -p e2e local_node_no_liquidity_limit_order --test-threads 1 --failure-output final --run-ignored ignored-only; then
exit 1
fi
attempt=$((attempt+1))
done
3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ The CI (check .github/workflows/pull-request.yaml) runs unit tests, e2e tests, `

`cargo clippy --all-features --all-targets -- -D warnings`

### Flaky Tests
In case a test is flaky and only fails **sometimes** in CI you can use the [`run-flaky-test`](.github/workflows/pull-request.yaml) github action to test your fix with the CI to get confidence that the fix that works locally also works in CI.

## Development Setup

### Postgres
Expand Down
68 changes: 34 additions & 34 deletions crates/e2e/tests/e2e/limit_orders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ async fn local_node_limit_does_not_apply_to_in_market_orders_test() {
run_test(limit_does_not_apply_to_in_market_orders_test).await;
}

#[tokio::test]
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
#[ignore]
async fn local_node_no_liquidity_limit_order() {
run_test(no_liquidity_limit_order).await;
Expand Down Expand Up @@ -787,27 +787,47 @@ async fn no_liquidity_limit_order(web3: Web3) {
.await
.unwrap_err();

// Create liquidity
onchain
.seed_weth_uni_v2_pools([&token_a].iter().copied(), to_wei(1000), to_wei(1000))
.await;

// Drive solution
tracing::info!("Waiting for trade.");
let balance_before = onchain
.contracts()
.weth
.balance_of(trader_a.address())
.call()
.await
.unwrap();
wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 1 })
.await
.unwrap();

wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 0 })
.await
.unwrap();
// Create liquidity
onchain
.seed_weth_uni_v2_pools([&token_a].iter().copied(), to_wei(1000), to_wei(1000))
.await;

// Drive solution
tracing::info!("Waiting for trade.");

// Keep minting blocks to eventually invalidate the liquidity cached by the
// driver making it refetch the current state which allows it to finally compute
// a solution.
for _ in 0..20 {
onchain.mint_block().await;
}

// wait for trade to be indexed
wait_for_condition(TIMEOUT, || async {
!services.get_trades(&order_id).await.unwrap().is_empty()
})
.await
.unwrap();

let trade = services.get_trades(&order_id).await.unwrap().pop().unwrap();
let fee = trade.executed_protocol_fees.first().unwrap();
assert_eq!(
fee.policy,
model::fee_policy::FeePolicy::Surplus {
factor: 0.5,
max_volume_factor: 0.01
}
);
assert_eq!(fee.token, onchain.contracts().weth.address());
assert!(fee.amount > 0.into());

let balance_after = onchain
.contracts()
Expand All @@ -817,24 +837,4 @@ async fn no_liquidity_limit_order(web3: Web3) {
.await
.unwrap();
assert!(balance_after.checked_sub(balance_before).unwrap() >= to_wei(5));

let trades = services.get_trades(&order_id).await.unwrap();
let executed_protocol_fee = trades
.first()
.unwrap()
.executed_protocol_fees
.first()
.unwrap();
assert_eq!(
executed_protocol_fee.policy,
model::fee_policy::FeePolicy::Surplus {
factor: 0.5,
max_volume_factor: 0.01
}
);
assert_eq!(
executed_protocol_fee.token,
onchain.contracts().weth.address()
);
assert!(executed_protocol_fee.amount > U256::zero());
}

0 comments on commit 6c73eb6

Please sign in to comment.