Integration tests: utxo-filter, utxo-consolidation, create-nft #164

tuliomir · 2022-03-22T22:52:33Z

This pull request is the third phase of the implementation of Integration Tests on the Hathor Wallet Headless, and expands the scope of #149 and #152.

Acceptance Criteria

Run the integration tests on the following routes:
- /wallet/utxo-filter
- /wallet/utxo-consolidation
- /wallet/create-nft

Other changes

Improves JSDocs for existing helpers
Improves error handling
- Improves the helpers' error handling
- GitHub workflow log output with better error description
Improves logging
- Adds monitoring on wallet init
- Logs for the dockerized private network added on the run artifacts
Centralizes the code for waiting on Websocket update on the helper methods
- A few tests that call a URL directly to make a transaction still have the delay explicitly
- Cleaner code
- More stable runs on GitHub CI

Security Checklist

Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

codecov · 2022-03-22T22:55:59Z

Codecov Report

Merging #164 (a9225bb) into dev (4fc71df) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev     #164   +/-   ##
=======================================
  Coverage   85.78%   85.78%           
=======================================
  Files          19       19           
  Lines         647      647           
  Branches      132      132           
=======================================
  Hits          555      555           
  Misses         83       83           
  Partials        9        9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fc71df...a9225bb. Read the comment docs.

r4mmer · 2022-03-31T01:15:35Z

.github/workflows/integration-test.yml

+
+      - name: Collect docker logs on failure
+        if: failure()
+        uses: jwalton/gh-docker-logs@v1


I like this action to make an artifact with the logs on failure but isn't there any official action for this? Could we fork it as to not depend on this external action?

On a quick search I found no official action for this purpose.

The fact that this one is announced on the GitHub Action Marketplace gives me some confidence that this code will be running safely in the future, but I agree that a fork would be a safe solution as well.

Maybe we could open an issue dedicated to make this?

__tests__/integration/balance.test.js

r4mmer · 2022-03-31T01:19:49Z

__tests__/integration/configuration/test.config.js

@@ -14,5 +14,5 @@ module.exports = {
  wsUpdateDelay: process.env.TEST_WS_UPDATE_DELAY || 1000,

  // Defines for how long the startMultipleWalletsForTest can run
-  walletStartTimeout: process.env.TEST_WALLET_START_TIMEOUT || 60000,
+  walletStartTimeout: process.env.TEST_WALLET_START_TIMEOUT || 180000,


I know this is the default, but I think we should add this variable on the github config when running the CI.
This way we can change the value when we want (or for a batch of tests).

The environment variable was added to GitHub on f8d62f2.

__tests__/integration/address-info.test.js

r4mmer · 2022-03-31T01:34:14Z

__tests__/integration/create-nft.test.js

+      expect(nftErr).toBeInstanceOf(Error);
+      expect(nftErr.response.text).toContain(field);
+    }
+    done();


I think we can just not use the done when we don't need it.

it('should etc', async () => { // test body });

ref

Tests on the 3 suites within the scope of this PR were rewritten without the done() call

r4mmer · 2022-03-31T01:46:40Z

__tests__/integration/create-nft.test.js

+    expect(nftTx.hash).toBeDefined();
+
+    // Validating authority tokens
+    const authorityOutputs = nftTx.outputs.filter(o => o.token_data === TOKEN_DATA.AUTHORITY_TOKEN);


I saw that TOKEN_DATA.AUTHORITY_TOKEN is 129, which works because we only have 1 custom token on this tx but I don't think we should do it like this.
Instead, use the authority mask (128) and filter with the & operator this will make sure this works even if we include multiple tokens on a transaction.

Great point! This would give a headache for a future test developer.
Fixed this on a1431c7

r4mmer · 2022-03-31T01:49:41Z

__tests__/integration/utils/test-utils-integration.js

+export const TOKEN_DATA = {
+  HTR: 0,
+  TOKEN: 1,
+  AUTHORITY_TOKEN: 129


We should use authority mask instead of assuming we only have 1 token on the tx.

Applied the wallet-lib constants mask on 7ada0f6

r4mmer · 2022-03-31T12:48:38Z

__tests__/integration/utils/wallet-helper.js

        TestUtils.logError(errMsg);
+        startBenchmark.failureAt = timestamp;
+        startBenchmark.failureDiff = failureDiff;
+        TestUtils.logTx(`Wallet init failure`, startBenchmark);


Should we change logTx to just log?

Done in 765442b

r4mmer · 2022-03-31T14:20:52Z

jest-integration.config.js

+const mainTestMatch = process.env.SPECIFIC_INTEGRATION_TEST_FILE
+  ? `<rootDir>/__tests__/integration/**/${process.env.SPECIFIC_INTEGRATION_TEST_FILE}.test.js`
+  : '<rootDir>/__tests__/integration/**/*.test.js'


Sugestion: take this from command line arguments so we can run specific tests with npm run test_network_integration -- start for instance

Suggested change

const mainTestMatch = process.env.SPECIFIC_INTEGRATION_TEST_FILE

? `<rootDir>/__tests__/integration/**/${process.env.SPECIFIC_INTEGRATION_TEST_FILE}.test.js`

: '<rootDir>/__tests__/integration/**/*.test.js'

const args = process.argv.slice(6);

const testMatch = args.length == 0 ? ['<rootDir>/__tests__/integration/**/*.test.js'] : args.map(arg => `<rootDir>/__tests__/integration/**/${arg}.test.js`);

Obs: the slice(6) is because we have "node", the filename and 4 other arguments we pass to jest on package.json

Edit: actually, we can already fillter tests with the command line wihtout this.
Running npm run test test_network_integration -- address will match all tests on __tests__/integration/ but will filter with the pattern /address/.

I can confirm it already has this behavior. You can even inform an environment variable and the command line - the later filters the first.

I prefer keeping it that way, so that I can use not only the test_network_integration script but also the test_integration that derives from it.

r4mmer · 2022-03-31T15:23:16Z

During my review I found it somewhat confusing when using helper methods to test the endpoints.
I think we should refactor these tests to make the call to the endpoint being tested directly from the test and use the helpers only to build the test scenario (and cleanup if necessary)

tuliomir · 2022-03-31T22:01:00Z

I think we should refactor these tests to make the call to the endpoint being tested directly from the test and use the helpers only to build the test scenario (and cleanup if necessary)

A refactor was made to implement this on the three test suites that are in the scope of this PR.

The test code is more straightforward now and doubles as documentation, as expected from tests. The only issue being that we have to remember to manually put pauses for websocket update where needed.

.github/workflows/integration-test.yml

__tests__/integration/utils/wallet-helper.js

__tests__/integration/utils/test-utils-integration.js

__tests__/integration/create-nft.test.js

__tests__/integration/utxo-filter.test.js

# Conflicts: # __tests__/integration/txLogger.js # __tests__/integration/utils/test-utils-integration.js # __tests__/integration/utils/wallet-helper.js

# Conflicts: # .github/workflows/integration-test.yml

__tests__/integration/configuration/config-fixture.js

__tests__/integration/utils/wallet-helper.js

__tests__/integration/utxo-filter.test.js

tuliomir added the tests label Mar 22, 2022

tuliomir self-assigned this Mar 22, 2022

tuliomir requested a review from r4mmer March 23, 2022 21:03

tuliomir marked this pull request as ready for review March 23, 2022 21:03

tuliomir requested a review from pedroferreira1 as a code owner March 23, 2022 21:03

tuliomir marked this pull request as draft March 25, 2022 23:18

tuliomir added 8 commits March 30, 2022 12:45

chore: improvements on logging, helper functions and configurations

93a7598

chore: moves mining to separate wallet, to improve genesis loading times

38b5c4a

test: utxo-filter tests

da798fb

test: utxo-consolidation tests

69dc938

test: create-nft tests

dd17879

perf: removing redundant delays

205d12b

perf: adding centralized delay for createNft

ad50a0c

ci: fixes branch name on workflow

c6dd570

tuliomir force-pushed the test/integration-3 branch from 5d9ed6b to c6dd570 Compare March 30, 2022 22:33

ci: remove branch name from workflow. Working with dev PR now.

51e67ee

tuliomir marked this pull request as ready for review March 30, 2022 22:53

r4mmer requested changes Mar 31, 2022

View reviewed changes

tuliomir added 9 commits March 31, 2022 12:45

fix: authority output validation rule

a1431c7

refactor: logTx method renamed to log

765442b

ci: adds wallet start timeout environment variable on GitHub workflow

f8d62f2

refactor: reusing authority mask from lib

7ada0f6

refactor: testing create-nft route directly via http requests

69de885

refactor: testing utxo-filter route directly via http requests

a500b37

refactor: testing utxo-consolidation route directly via http

5190bc5

fix: minor fixes to delays and change address

eb666cb

fix: more stability delays

9530714

pedroferreira1 reviewed Apr 4, 2022

View reviewed changes

tuliomir added 4 commits April 4, 2022 12:12

refactor: reusing more of the wallet-lib code + minor fixes

740ecc7

test: asserting nft fee on first output. Minor fix on sample data.

200f4b7

test: asserting utxo filter max_utxos result compared with unfiltered

5f47561

Merge branch 'dev' into test/integration-3

f9b8aa1

# Conflicts: # __tests__/integration/txLogger.js # __tests__/integration/utils/test-utils-integration.js # __tests__/integration/utils/wallet-helper.js

r4mmer approved these changes Apr 5, 2022

View reviewed changes

tuliomir added 2 commits April 5, 2022 12:55

test: increased timeout

be62c78

perf: option to serialize wallet start

c69f1b4

tuliomir mentioned this pull request Apr 8, 2022

Test APIs that changes the wallet balance/history #88

Closed

tuliomir added 5 commits April 8, 2022 16:51

Merge branch 'dev' into test/integration-3

12dd104

# Conflicts: # .github/workflows/integration-test.yml

fix: miner wallet init with obsolete parameters

d16e34b

fix: minor fixes for dev merge issues / linting

eb0760c

test: skipping multisig tests that fail

ab4fa7d

style: lint fixes

5338fd1

tuliomir mentioned this pull request Apr 11, 2022

Fix the issues on skipped tests #161

Open

3 tasks

pedroferreira1 reviewed Apr 12, 2022

View reviewed changes

tuliomir added 2 commits April 12, 2022 17:58

refactor: improved variable names and added more jsdocs to explain them

7c199cd

docs: comment block explaining why the websocket timeout is being set

10e0b6b

r4mmer approved these changes Apr 14, 2022

View reviewed changes

tuliomir added 4 commits April 14, 2022 13:42

fix: removes obsolete "#started" validation that was causing bugs

015a196

perf: removes redundant waiting on wallet ready

7fa1121

docs: improves connectionTimeout explanatory comment

988283f

fix: fetching correct timestamp to measure wallet init timeout

a9225bb

pedroferreira1 self-requested a review April 14, 2022 22:38

pedroferreira1 approved these changes Apr 14, 2022

View reviewed changes

tuliomir merged commit 7a29078 into dev Apr 14, 2022

tuliomir deleted the test/integration-3 branch April 14, 2022 22:53

This was referenced Apr 19, 2022

bump: v0.12.0 #180

Merged

Release: v0.12.0 #182

Merged

tuliomir mentioned this pull request May 4, 2022

fix: skipped multisig tests #191

Closed

Integration tests: utxo-filter, utxo-consolidation, create-nft #164

Integration tests: utxo-filter, utxo-consolidation, create-nft #164

Uh oh!

Conversation

tuliomir commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Acceptance Criteria

Other changes

Security Checklist

Uh oh!

codecov bot commented Mar 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r4mmer commented Mar 31, 2022

Uh oh!

tuliomir commented Mar 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tuliomir commented Mar 22, 2022 •

edited

Loading

codecov bot commented Mar 22, 2022 •

edited

Loading