Define a proper base class for fixture tests by pm47 · Pull Request #2286 · ACINQ/eclair

pm47 · 2022-05-27T17:27:36Z

This PR does two main things:

introduce a new FixtureSpec base class for tests that involve a fixture. See the scaladoc for more info. The existing test WaitForOpenChannelStateSpec has been migrated as an example. The diff is small but we separate clearly the fixtures from the test framework. There is duplication for now, but the goal is for ChannelStateTestsHelperMethods to disappear.
add new simple integration tests in package integration.basic. They are based on MinimalNodeFixture, which is a full setup for a node with real actors, except the bitcoin part (watcher/wallet) which is mocked. They are much lighter than our previous integration tests, which allow us to keep each test individual, as opposed to having all tests of the same suite depend on each other. We can define more complex fixtures with any number of nodes.

Also updated scalatest version and fixed a few issues.

codecov-commenter · 2022-05-27T19:37:11Z

Codecov Report

Merging #2286 (2e6e566) into master (44e7127) will decrease coverage by 0.03%.
The diff coverage is 82.35%.

@@            Coverage Diff             @@
##           master    #2286      +/-   ##
==========================================
- Coverage   84.40%   84.36%   -0.04%     
==========================================
  Files         194      194              
  Lines       14411    14419       +8     
  Branches      567      581      +14     
==========================================
+ Hits        12163    12164       +1     
- Misses       2248     2255       +7

Impacted Files	Coverage Δ
...main/scala/fr/acinq/eclair/io/PeerConnection.scala	`85.55% <50.00%> (-0.43%)`	⬇️
...air-core/src/main/scala/fr/acinq/eclair/Logs.scala	`83.33% <100.00%> (+0.28%)`	⬆️
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala	`88.39% <100.00%> (+0.11%)`	⬆️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala	`90.41% <100.00%> (ø)`
...in/scala/fr/acinq/eclair/io/ReconnectionTask.scala	`98.07% <100.00%> (+1.92%)`	⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala	`93.78% <100.00%> (+0.03%)`	⬆️
...q/eclair/channel/publish/ReplaceableTxFunder.scala	`86.91% <0.00%> (-3.67%)`	⬇️
...src/main/scala/fr/acinq/eclair/db/pg/PgUtils.scala	`89.39% <0.00%> (-1.52%)`	⬇️
...cinq/eclair/payment/receive/MultiPartHandler.scala	`93.61% <0.00%> (-1.42%)`	⬇️
... and 4 more

t-bast

I like the direction in which we're going with that change, and I'm happy to merge that PR and start using these new fixtures for new code, but before migrating old code or removing ChannelStateTestHelperMethods we should really integrate dual-funding which touches those a lot, otherwise we'll make a complex set of PRs even more complex to integrate!

t-bast · 2022-05-29T22:38:42Z

+  override def makeFundingTx(pubkeyScript: ByteVector, amount: Satoshi, feeRatePerKw: FeeratePerKw)(implicit ec: ExecutionContext): Future[MakeFundingTxResponse] = {
+    val tx = DummyOnChainWallet.makeDummyFundingTx(pubkeyScript, amount)
+    funded += (tx.fundingTx.txid -> tx.fundingTx)
+    Future.successful(tx)


I'm making many changes to these test helpers in #2275, if you plan on making more changes here in follow-up PRs we should probably do it after dual-funding to minimize conflicts.

Gotcha, I don't plan to make more changes so we should be fine. Dealing with the mempool/blockchain is one of the trickiest parts, because they need to be shared among various MinimalNodeFixture. I have explored several approaches and am still not 100% happy.

In particular, there are two ways of confirming channels: MinimalNodeFixture.confirmChannel and MinimalNodeFixture.autoConfirmLocalChannels. I think the latter (based on autopilot) is nicer when you don't need fine-grained control, but the core issue of having access to the actual transaction remains. For example, note that we pass a reference to alice's wallet for bob's autopilot:

eclair/eclair-core/src/test/scala/fr/acinq/eclair/integration/basic/TwoNodesIntegrationSpec.scala

Line 50 in 2e6e566

bob.watcher.setAutoPilot(autoConfirmLocalChannels(alice.wallet.funded))

Maybe with dual-funding this will be a tad simpler since both sides know the full tx?

pm47 · 2022-05-30T10:22:38Z

I like the direction in which we're going with that change, and I'm happy to merge that PR and start using these new fixtures for new code, but before migrating old code or removing ChannelStateTestHelperMethods we should really integrate dual-funding which touches those a lot, otherwise we'll make a complex set of PRs even more complex to integrate!

The WaitForOpenChannelStateSpec migration doesn't bring much by itself. I can either revert it, or migrate all other tests. It might be worth it to migrate the rest because it should reduce resource consumption, but OTOH maybe better to do it properly in a dedicated PR and take the time to improve the migrated tests. Or I can do a quick migration and we improve them later. WDYT?

edit: On 2nd thought, maybe better to wait for the akka typed migration before revamping the channel tests.

pm47 · 2022-05-30T12:24:38Z

 * Created by PM on 11/03/2020.
 */
-class PeerConnection(keyPair: KeyPair, conf: PeerConnection.Conf, switchboard: ActorRef, router: ActorRef) extends FSMDiagnosticActorLogging[PeerConnection.State, PeerConnection.Data] {
+class PeerConnection(keyPair: KeyPair, conf: PeerConnection.Conf, switchboard: ActorRef, router: ActorRef) extends FSMDiagnosticActorLogging[PeerConnection.State, PeerConnection.Data] with Stash {


Another pretty good reason not to use TestActorRef is that it is not compatible with Stash (https://stackoverflow.com/a/18377939).

Stash allows us to nicely handle race conditions between messages, instead of delaying them, which creates other race conditions (which is exactly what happened in my attempted fix 2632be3).

pm47 · 2022-05-30T12:25:20Z

      }
      d.transport ! localInit
      startSingleTimer(INIT_TIMER, InitTimeout, conf.initTimeout)
+      unstashAll() // unstash remote init if it already arrived


NB: I'm only unstashing right before we are ready to process the remote init.

This reverts commit 3ef4f44.

t-bast · 2022-05-31T22:20:56Z

It might be worth it to migrate the rest because it should reduce resource consumption, but OTOH maybe better to do it properly in a dedicated PR and take the time to improve the migrated tests. Or I can do a quick migration and we improve them later. WDYT?

If we can delay the migration of the channel tests just after we do dual funding, that would be great :)
Let's get this PR merged and the new fixtures used for new tests, and we start migrating old tests right after we've dealt with dual funding?

pm47 · 2022-06-01T08:09:43Z

If we can delay the migration of the channel tests just after we do dual funding, that would be great :)

Agreed, done in fb2e439.

The circular dependency to TestKitBaseClass/TestKitBase was really weird, we had: - NormalStateSpec -> TestKitBaseClass -> ChannelStateTestsBase - ChannelStateTestsBase -> ChannelStateTestsHelperMethods - ChannelStateTestsHelperMethods -> TestKitBase

Triple equals allows customizing the equality method, but we never use that. Detailed errors (which is what we were looking for) are provided by mixing in scalatest's `Assertions` and using the regular `assert(x==y)` syntax. The `Assertions` trait is already mixed in by default in all scalatest suites.

pm47

I did some cleanup which shouldn't impact your dual-funding tests.

pm47 · 2022-06-01T15:37:46Z

    bob2blockchain.expectMsgType[WatchFundingDeeplyBuried]
-    awaitCond(alice.stateName == NORMAL)
-    awaitCond(bob.stateName == NORMAL)
+    eventually(assert(alice.stateName == NORMAL))


Before:

assertion failed: timeout 149050696200 nanoseconds expired: java.lang.AssertionError: assertion failed: timeout 149050696200 nanoseconds expired:

After:

The code passed to eventually never returned normally. Attempted 9 times over 161.3674 milliseconds. Last failure message: NORMAL did not equal CLOSED. ScalaTestFailureLocation: fr.acinq.eclair.channel.states.ChannelStateTestsHelperMethods at (ChannelStateTestsHelperMethods.scala:245) Expected :CLOSED. Actual :The code passed to eventually never returned normally. Attempted 9 times over 161.3674 milliseconds. Last failure message: NORMAL

No comment!

pm47 added 8 commits May 23, 2022 17:07

update test dependencies

e708618

add node alias to log MDC for tests

f28bedd

proper base class for fixture tests

0428509

adapt WaitForOpenChannelStateSpec to new fixture

3ef4f44

fix race condition in peerconnection

2632be3

human-readable nodeId for alice/bob

293725d

fix sqlite db roundtrip check

f175570

use eclair default params in integration tests

2e6e566

pm47 requested a review from t-bast May 27, 2022 17:27

t-bast reviewed May 29, 2022

View reviewed changes

address review comments

3d42e3a

fixup! fix race condition in peerconnection

3376a93

pm47 commented May 30, 2022

View reviewed changes

pm47 added 2 commits May 30, 2022 14:26

Revert "adapt WaitForOpenChannelStateSpec to new fixture"

fb2e439

This reverts commit 3ef4f44.

fixup! address review comments

ffeb8e5

pm47 requested a review from t-bast May 30, 2022 12:38

pm47 added 3 commits June 1, 2022 17:35

merge ChannelStateTestsBase and ChannelStateTestsHelperMethods

2da5a97

pm47 commented Jun 1, 2022

View reviewed changes

t-bast approved these changes Jun 2, 2022

View reviewed changes

pm47 merged commit 9610fe3 into master Jun 3, 2022

pm47 deleted the simple-integration branch June 3, 2022 07:58

t-bast mentioned this pull request Jun 6, 2022

Implement first steps of the dual funding flow #2247

Merged

Conversation

pm47 commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented May 27, 2022

Codecov Report

Uh oh!

t-bast left a comment

Choose a reason for hiding this comment

Uh oh!

t-bast May 29, 2022

Choose a reason for hiding this comment

Uh oh!

pm47 May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pm47 commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pm47 May 30, 2022

Choose a reason for hiding this comment

Uh oh!

pm47 May 30, 2022

Choose a reason for hiding this comment

Uh oh!

t-bast commented May 31, 2022

Uh oh!

pm47 commented Jun 1, 2022

Uh oh!

pm47 left a comment

Choose a reason for hiding this comment

Uh oh!

pm47 Jun 1, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pm47 commented May 27, 2022 •

edited

Loading

pm47 May 30, 2022 •

edited

Loading

pm47 commented May 30, 2022 •

edited

Loading