Collect system-level tests and start removing Ruby tests

[abailly]

This PR is a first step in an effort to rationalise our testing approach as outlined in #4852.

• list existing integration tests and e2e tests side-by-side
• remove misc Ruby tests which are not testing cardano-wallet

[paweljakubas]

👍

If I understand well we will need to remember whenever we add/remove any integration test we need to update tests.md

[abailly]

Yes, that's more or less the idea.

[abailly]

@paweljakubas Given that you seem to be the one most aware about those integration tests, could you have a look at the ruby tests and mark which ones, in your opinion we should preserve?

[paweljakubas]

@abailly not a question I can give 100% confidence answer. why? Those tests were added long time ago by our QA engineer (that happened to know Ruby) and were supposed to replicate some integration tests and run them against preprod (previously testnet) and check scenarios engaging wallets with HISTORY. Ocassionally, they were able to surface edge case bugs. As we are not adding logic and integration tests so much and Ruby tests were run many times and found what they can detect, AND as we are not so eager to work with this codebase, I would tentatively propose to abandon them. If we are to start adding integration tests + logic we might return to the subject and think how to test against preprod with wallet with history but with codebase we are into. @HeinrichApfelmus @paolino @Anviking make sense?

[paweljakubas]

to sum up: I would remove them all as we speak as they are burden now and not adding value imho.

[abailly]

Thanks @paweljakubas, I am happy to remove those, but let's wait for feedback from the rest of the team

[HeinrichApfelmus]

It appears that this test exercises the POST /proxy/transactions endpoint of cardano-wallet.

[Anviking]

These are currently no longer tested with integration tests: https://github.com/cardano-foundation/cardano-wallet/blob/master/lib/integration/scenarios/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs#L2987-L3045 (withdrawal not there, the others exist but are marked pending)

[HeinrichApfelmus]

I would like to note that — most unfortunately — I find the Ruby E2E tests easier to read and understand than the Haskell integration tests.

• One of the main reasons for the increased complexity of the Haskell integration tests is that the they are mostly based on the Cardano.Wallet.Api.Link module, which allows sending malformed data to endpoints.

  cardano-wallet/lib/api/src/Cardano/Wallet/Api/Link.hs

  Lines 16 to 23 in a9f941c

  	-- Type-safe endpoint accessors for the wallet API. Under normal circumstances,
  	-- one would prefer to use 'WalletClient' from 'Cardano.Wallet.Api.Client' and
  	-- not to bother with endpoints at all.
  	--
  	-- Yet, in some cases (like in black-box testing), one could want to purposely
  	-- send malformed requests to specific endpoints. Thus, this module facilitates
  	-- the construction of valid endpoints that'd be accepted by the server, and for
  	-- which, users are free to send all sort of data as payload, valid or invalid.

  I'm not sure that it's a good idea to base a large test suite on this — ideally, the concern of malformed data is resolved by unit testing decoder / encoders.

• In the absence of specialized types for requests and response, good syntactic support for general JSON types would be helpful to make the tests readable. Ruby has good syntactic support for inspecting JSON types, e.g. res['network_info']['protocol_magic'], at the usual price of partial functions. However, we never invested into similar syntactic amenities for Haskell.

To summarize: Unfortunately, our Haskell integration tests contain technical debt to the point where I think that they do not compete well against the Ruby E2E tests.

[abailly]

@HeinrichApfelmus From what I have seen, I am tempted to agree and I am against building such a large integration test suite as it's a great source of tech debt, complexity, flaky tests, etc.
But we need to do something as we probably don't want to maintain 2 set of mostly overlapping slow-to-run-hard-to-debug test suite.

[HeinrichApfelmus]

But we need to do something as we probably don't want to maintain 2 set of mostly overlapping slow-to-run-hard-to-debug test suite.

Yes. The question is of course: "Which something?"

The way I see is that we have different options with different amounts of effort:

1. Maintain 2 sets of mostly overlapping slow-to-run-hard-to-debug test suites.
2. Rewrite Ruby E2E tests to disappear.
3. Rewrite Haskell integration tests to become more readable and competitive with the Ruby E2E tests.
4. Rewrite Haskell integration tests to become unit tests and remove matching Ruby E2E tests.
5. …

It is not clear to me that 1. is a priori more effort than 2, 3, or 4. In addition, we have to consider the opportunity cost of not investing into option

6. extend the "fresh start" of the "Deposit Wallet" to include features from the legacy wallet, mostly related to delegation.

The modular our codebase is, the cheaper 6. becomes. Option 4. would move us to a more modular codebase, so my gut feeling is that a combination of 4 and 6 plus down-payments on 1 is probably the route that requires less engineering effort than the others.

[abailly]

My comments (but I really think we need to spend more time discussing f2f in front of whiteboard than through GH clumsy PR):

1. Maintain 2 sets of mostly overlapping slow-to-run-hard-to-debug test suites.

I don't mind having Ruby code, I just don't want to maintain 2 different things which serve the same purpose and each take time to run, delay feedback, are hard to run locally, and ultimately hamper delivery speed.
So we definitely want to remove the overlap.

3. Rewrite Haskell integration tests to become more readable and competitive with the Ruby E2E tests.

Significant effort, and I already said I would rather reduce the amount of integration tests we have as I am 100% confident we are testing things which would be much better tested at a finer grained level.

4. Rewrite Haskell integration tests to become unit tests and remove matching Ruby E2E tests.

That has my favour.

6. extend the "fresh start" of the "Deposit Wallet" to include features from the legacy wallet, mostly related to delegation.

I don't understand what this has to do with the discussion.

This PR's goal is to spark the discussion and start the ball rolling, but what I am sure is that statu quo is undesirable. It's unclear to me what value do the E2E tests bring us given the apparently huge amount of overlap they have with Haskell integration tests and the latter seem much more actively maintained (and perhaps that's a problem too).

Quick stats:

% git log --oneline  --since 2024-01-01 lib/integration | wc -l
     194
% git log --oneline --since 2024-01-01 test/e2e | wc -l
      44

I would like to also understand what's the payoff for all these system-level tests, and I understand this is hard to tell: How many times did the E2E test suite prevented a change from introducing a bug? Same question goes for Haskell integration tests?

[HeinrichApfelmus]

My comments (but I really think we need to spend more time discussing f2f in front of whiteboard than through GH clumsy PR):

Sure. Perhaps put it on the agenda for our Dev Meeting?

So we definitely want to remove the overlap.

I don't disagree, but I think that the "definitely" needs to be weighted by effort compared to effort of alternatives.

6. extend the "fresh start" of the "Deposit Wallet" to include features from the legacy wallet, mostly related to delegation.

I don't understand what this has to do with the discussion.

What I'm trying to say is that the effort of maintaining a test suite only pays off it the software under test is still subject to meaningful changes. At this point, it's not clear to me that this is still the case. Hence I'm bringing up the alternative of freezing the old in cardano-wallet and its test suite in place (I'm not necessarily in favor of this, but do think that it's realistic enough to bring up.)

I would like to also understand what's the payoff for all these system-level tests

In the past, these have been the only tests for features such as delegating voting power to a DRep.

How many times did the E2E test suite prevented a change from introducing a bug?

Pawel and Johannes know better, but I am told that historically, Piotr's E2E tests have caught at least three bugs.

[abailly]

Here is a quick summary of current situation:

• I don't have the depth of knowledge of c-w to be confident in my judgment but I would be tempted to kill those tests
• @HeinrichApfelmus is unsure whether or not this tests are properly replaced by other tests and is in favour of statu quo and not invest time on this tests given our current focus on deposit wallet
• @paweljakubas is in favour of removing Ruby ETE tests altogether
• @Anviking is not sure

What should we do?

[abailly]

It seems attempting a smooth and incremental migration is not really worth it, so we are left with 2 options:

• Keep them as is
• Completely remove them

[HeinrichApfelmus]

I just realized that the Ruby E2E tests are our only tests that exercise the Windows platform. If we remove them now, we don't have any integration test on Windows before a release. 🤔

[abailly]

That's a fair point @HeinrichApfelmus