Cleanup integration tests #757

markspanbroek · 2024-03-26T13:27:54Z

Moves the tests from the venerable testIntegration.nim into several new test modules.

testrestapi.nim: tests that exercise and test the REST API
testupdownload.nim: tests that exercise uploading and downloading content
testpurchasing.nim: tests for buying storage
testsales.nim: tests for selling storage
testmarketplace.nim: tests that exercise a combination of buying and selling storage

Part of #746

emizzle

LGTM, except oddly there is a failing test 🤔

markspanbroek · 2024-03-28T15:27:31Z

Failing test should be fixed in e62843d. Timing between test runner ( provider.getCurrentTime() ) and nodes ( clock.now ) was sometimes off by more than 10 seconds, failing the test.

I ran the test 100x, and it succeeded all the time.

dryajov

LGTM

emizzle

What are your thoughts on moving some of these tests over from twonodesuite to marketplacesuite?

To ensure that tests involving multiple nodes do not start with out-of-sync clocks

markspanbroek · 2024-05-21T07:01:48Z

What are your thoughts on moving some of these tests over from twonodesuite to marketplacesuite?

I would say that that would be part of the work in #746, but not for this PR.

markspanbroek · 2024-05-21T07:07:50Z

The intermittently failing test should be fixed in 97a57a1. The test would fail when the storage request was cancelled while a proof was being generated. The async state machine would then cancel the proving state, which triggers a chronos cancellation of future that calculates the proof. But the code that calculates the proof would catch and ignore the CancelledError (by catching all CatchableErrors). This pattern of catching CatchableError and not dealing with CancelledError properly happens in more places in the codebase. I tried to fix them all.

emizzle · 2024-05-21T08:06:20Z

The intermittently failing test should be fixed in 97a57a1. The test would fail when the storage request was cancelled while a proof was being generated. The async state machine would then cancel the proving state, which triggers a chronos cancellation of future that calculates the proof. But the code that calculates the proof would catch and ignore the CancelledError (by catching all CatchableErrors). This pattern of catching CatchableError and not dealing with CancelledError properly happens in more places in the codebase. I tried to fix them all.

Right ok, this is a great catch! Just to clarify, you mean the initialproving state, correct? This is where the initial proof is generated, in the node.onProve callback, and if cancelled, would be caught here: 97a57a1#diff-1809bdd70926fdcad64f23f0974984916e79885ea5316ed2f2ac1457583bb3e5R701-R702

markspanbroek · 2024-05-21T11:06:34Z

Just to clarify, you mean the initialproving state, correct?

No, if I remember correctly it was in the proving state. It had already filled the slot, but the other slots in the contract do not get filled, so eventually the contract is cancelled. This is also why the test was failing intermittently; the odds of it having to provide a proof at the same time that the storage contract expires is small.

dryajov · 2024-05-21T23:28:41Z

codex/blockexchange/network/network.nim

@@ -244,7 +246,8 @@ proc rpcHandler(

    if payment =? SignedState.init(msg.payment):
      asyncSpawn b.handlePayment(peer, payment)
-
+  except CancelledError as error:


If we're going to asyncSpawn handlePayment, there is no point in try/catch around it. Any escaping exception is converted to a Defect by asyncSpawn.

Good point, I have no idea why the error handling was here in the first place. I just added the extra case for CancelledError, but in the current code everything is asyncSpawned, and no errors will be raised anyway.

dryajov · 2024-05-21T23:31:39Z

codex/blockexchange/network/networkpeer.nim

@@ -72,6 +74,8 @@ proc broadcast*(b: NetworkPeer, msg: Message) =
  proc sendAwaiter() {.async.} =
    try:
      await b.send(msg)
+    except CancelledError as error:
+      raise error


Don't re-raise in a proc that is asyncSpawn it will be terminate the application because it is converted to a Defect. Anything that is asyncSpawn should not raise exceptions.

Good catch, I misread the except as being part of broadcast instead of sendAwaiter.

By the way, it seems that broadcast is unused? Should we remove it?

dryajov · 2024-05-21T23:34:15Z

codex/chunker.nim

@@ -90,6 +90,8 @@ proc new*(
        res += await stream.readOnce(addr data[res], len - res)
    except LPStreamEOFError as exc:
      trace "LPStreamChunker stream Eof", exc = exc.msg
+    except CancelledError as error:
+      raise error
    except CatchableError as exc:
      trace "CatchableError exception", exc = exc.msg
      raise newException(Defect, exc.msg)


Not sure why we're converting this to a Defect, should maybe be simply re-raised?

No idea either, I just left that part alone.

No Exceptions can occur, only Defects, because everything is asyncSpawned. Co-authored-by: Dmitriy Ryajov <dryajov@gmail.com>

Co-authored-by: Dmitriy Ryajov <dryajov@gmail.com>

AuHau

Great catch! LGTM 👍

markspanbroek mentioned this pull request Mar 26, 2024

Integration tests improvements #746

Open

8 tasks

markspanbroek requested review from emizzle and AuHau March 26, 2024 13:32

emizzle previously approved these changes Mar 26, 2024

View reviewed changes

AuHau previously approved these changes Mar 27, 2024

View reviewed changes

markspanbroek force-pushed the cleanup-integration-tests branch from 3d07bbe to e62843d Compare March 28, 2024 15:32

markspanbroek enabled auto-merge March 28, 2024 15:34

markspanbroek added this pull request to the merge queue Mar 28, 2024

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 28, 2024

dryajov previously approved these changes Apr 2, 2024

View reviewed changes

markspanbroek dismissed stale reviews from AuHau and emizzle via e62843d May 14, 2024 16:02

markspanbroek dismissed dryajov’s stale review via 48091c0 May 16, 2024 06:39

markspanbroek force-pushed the cleanup-integration-tests branch from e62843d to 48091c0 Compare May 16, 2024 06:39

dryajov previously approved these changes May 16, 2024

View reviewed changes

dryajov enabled auto-merge May 16, 2024 17:19

markspanbroek disabled auto-merge May 20, 2024 10:57

emizzle previously approved these changes May 21, 2024

View reviewed changes

markspanbroek added 7 commits May 21, 2024 08:50

integration: move REST API tests into their own module

28f531e

integration: move upload and download tests into their own module

d4a015d

integration: move purchasing tests into their own module

7b8633b

integration: move marketplace tests to the right module

32e7c5e

integration: mine a block *after* starting nodes

0f7cdb9

To ensure that tests involving multiple nodes do not start with out-of-sync clocks

Fix: do not swallow CancellationErrors

97a57a1

integration: avoid underflow in UInt256

69fd071

markspanbroek dismissed stale reviews from emizzle and dryajov via 69fd071 May 21, 2024 06:50

markspanbroek force-pushed the cleanup-integration-tests branch from eceb914 to 69fd071 Compare May 21, 2024 06:50

dryajov reviewed May 21, 2024

View reviewed changes

markspanbroek and others added 2 commits May 22, 2024 09:38

network: remove unnecessary error handling

d210707

No Exceptions can occur, only Defects, because everything is asyncSpawned. Co-authored-by: Dmitriy Ryajov <dryajov@gmail.com>

network: do not raise in asyncSpawned proc

6b250c3

Co-authored-by: Dmitriy Ryajov <dryajov@gmail.com>

AuHau approved these changes May 22, 2024

View reviewed changes

dryajov approved these changes May 23, 2024

View reviewed changes

dryajov added this pull request to the merge queue May 23, 2024

Merged via the queue into master with commit 3046b76 May 23, 2024
10 checks passed

dryajov deleted the cleanup-integration-tests branch May 23, 2024 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup integration tests #757

Cleanup integration tests #757

markspanbroek commented Mar 26, 2024 •

edited

emizzle left a comment

markspanbroek commented Mar 28, 2024 •

edited

dryajov left a comment

emizzle left a comment

markspanbroek commented May 21, 2024

markspanbroek commented May 21, 2024

emizzle commented May 21, 2024

markspanbroek commented May 21, 2024

dryajov May 21, 2024

markspanbroek May 22, 2024

dryajov May 21, 2024

markspanbroek May 22, 2024

dryajov May 21, 2024

markspanbroek May 22, 2024

AuHau left a comment

Cleanup integration tests #757

Cleanup integration tests #757

Conversation

markspanbroek commented Mar 26, 2024 • edited

emizzle left a comment

Choose a reason for hiding this comment

markspanbroek commented Mar 28, 2024 • edited

dryajov left a comment

Choose a reason for hiding this comment

emizzle left a comment

Choose a reason for hiding this comment

markspanbroek commented May 21, 2024

markspanbroek commented May 21, 2024

emizzle commented May 21, 2024

markspanbroek commented May 21, 2024

dryajov May 21, 2024

Choose a reason for hiding this comment

markspanbroek May 22, 2024

Choose a reason for hiding this comment

dryajov May 21, 2024

Choose a reason for hiding this comment

markspanbroek May 22, 2024

Choose a reason for hiding this comment

dryajov May 21, 2024

Choose a reason for hiding this comment

markspanbroek May 22, 2024

Choose a reason for hiding this comment

AuHau left a comment

Choose a reason for hiding this comment

markspanbroek commented Mar 26, 2024 •

edited

markspanbroek commented Mar 28, 2024 •

edited