Improving integration tests

[janaakhterov]

We have many integration tests now which is fantastic, but they are quite fragile which makes it harder to rely on CI catching bugs. We've had test fail because of the following reasons:

• Receiving BUSY for all attempts on a request
• Functionality differing between previewnet and testnet for the same feature. Specifically we've had test fail because a different erring response code was returned than the one expected and/or no erring response code being returned when one was expected.
• Functionality changing for both previewnet and testnet which are not covered by the current tests. E.g. Setting an empty custom fee list on TokenFeeScheduleUpdate used to produce an error CUSTOM_SCHEDULE_ALREADY_HAS_NO_FEES, but now does not err.
• Test(s) running out of time. This issue mostly exists in JS since each test much be given a timeout, but does in a way occur in Go, although it's for the entire test suite instead of a per test basis.

Questions:

• How can we improve these integration tests to allow us to rely on CI more heavily?
• Should we set a very large maxAttempts value to lower the likeliness of BUSY causing an entire test to fail?
• Should there be a time limit on a per test basis so we don't have CI running for several hours?
• Should we have each test check if it's running against the right network (previewnet vs testnet)?
• How should we indicate a test is meant to run against integrationnet or should we do this at all?

[janaakhterov]

cc @Sean-Tedrow-LB @andrix10 @valtyr-naut @rbair23 @steven-sheehy @SimiHunjan

[steven-sheehy]

Have you heard of the test pyramid? The vast majority of your tests should be unit tests or integration tests. Your integration tests are actually considered E2E tests. E2E tests should be avoided in CI because of the reasons you mention but also because external contributors can't run them since they require secrets.

I think having E2E tests are valuable but they should probably be ran as a daily or periodic job and not part of CI that runs for PRs. The E2E tests should not be network specific. Instead, there should be some config which allows you to run them against different networks. Tests that only work in certain environments like new features should be feature flagged. Then you can construct config files per env that you could use to run the tests. Something like:

hedera:
  sdk:
    network: integration
    nodes:
      0.0.3: 1.2.3.4:50211
    mirrorNodes: [1.2.3.4:5600]
    feature:
      nft: false
      autoAssociation: true

You can use JUnit @EnabledIf/@DisabledIf to implement the feature flags. You can also use JUnit @Tag to break tests down by area.

[rbair23]

That is a great blog post, it is right on target.

[janaakhterov]

I was not aware of the testing pyramid before, but I'm glad I am aware of it now; it was a great blog post indeed. This does bring up the question of what should the SDK's tests really test? At the moment most of the tests within the SDKs are E2E tests because the current SDK testing goal is to guarantee the SDK works correctly for any given Hedera network; any addition to the SDK must also add some sort of relevant test that guarantees a request constructed from the SDK will result in the Hedera network returning some value. Removing these integration tests removes this guarantee; does that matter? In fact the SDKs have many tests now that expect a certain status code to be returned for a certain series of request; should that be something the SDKs test? If we were to shift the SDK's testing pyramid structure to look more like a pyramid then I think most of our tests would simply guarantee a certain request will serialize to a certain protobuf, and would not guarantee it works with any Hedera network. I believe the SDKs should have a reliable way of guaranteeing they work correctly with any Hedera Network, but I don't believe neither the current nor proposed testing pyramids guarantee this.

As for E2E tests being non-network specific and instead using a configuration file along with feature flags I'm still not completely sure if this really addresses the current issues within the SDK. I'm not saying having tests be feature flagged and feature gated is a bad thing, but I'm just not completely sold on feature flagging tests resolving the issue with functionality changing between the networks for the same feature. For example, when NFT support was added to previewnet it supported querying token NFT info by account ID and a serial number range. This feature made it to testnet as well making it consistent with previewnet. However, later this feature was removed from previewnet and soon after removed from testnet. For the brief moment it existed on testnet but not previewnet running our token NFT info query test that use an account ID would fail on previewnet but succeed on testnet; having this test feature gated for NFTs would not prevent this test from failing when run against previewnet. If we were to add another test which expects the token NFT info query to fail we would use the same feature flag which would result in one of these tests failing no matter if previewnet was chosen or testnet was chosen. If this feature ever comes back we'll have a similar issue because testnet would expect it to fail, but previewnet would expect it to succeed.

[steven-sheehy]

I don't think we want to remove the current E2E tests, but shift it to be acceptance tests that run periodically and before releases. It doesn't need to be comprehensive and cover every input option and error response. The services team already has a comprehensive test suit that verifies given protobuf X request it should produce protobuf Y response. You can keep it as comprehensive as you like, but point is it shouldn't be run for PRs since it's slow, brittle and requires secrets. I do think your main focus should be on unit tests and ensuring the SDK generates protobufs of the expected form and that you exercise all code paths as proved through coverage tools.

As for E2E tests being non-network specific and instead using a configuration file along with feature flags I'm still not completely sure if this really addresses the current issues within the SDK

I think it does and is in fact exactly how services handles the same problem. They use an api-permission.properties that controls what services are enabled and they have code that checks that and turns those on or off. In your NFT query scenario you'd already have some feature flags or tags around the query tests like nft, query and getTokenNftInfos. You'd have a nightly E2E matrix job that runs for each Hedera network you want to test against. Each job would have a separate config file (can even just be github environment variables) that controls what features are enabled. When the job for previewnet fails, you'd update its config to turn off the getTokenNftInfos flag and re-run it. The previewnet job now succeeds since that test is turned off and the testnet job succeeds since it is still on and enabled in the env.

The api-permission.properties is a file on the Hedera network. You can even get fancy and download it and use that to turn on and off features.

[SimiHunjan]

@rbair23 any comments here? how to proceed?