Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Organize unit tests and add integration tests with hsd #101

Merged
merged 7 commits into from
Nov 1, 2022

Conversation

pinheadmz
Copy link
Member

@pinheadmz pinheadmz commented Oct 14, 2022

Closes #23

This cleans up the framework for unit tests (written C and compiled into executable test_hnsd) and also introduces a new integration test system using nodejs and hsd.


--> ./test_hnsd
Testing hnsd...
test_base32
 test_base32_encode_decode
test_resource
 test_resource_pointer_to_ip
ok
--> cd integration/
--> npm run test

> [email protected] test
> bmocha --reporter spec test/*.js


  Basic sync & resolve
    ✓ should register names and sync (845ms)
    ✓ should resolve DS
    ✓ should resolve NS
    ✓ should resolve TXT
    ✓ should resolve GLUE4 without glue
    ✓ should resolve GLUE4 with glue
    ✓ should resolve GLUE6 without glue
    ✓ should resolve GLUE6 with glue
    ✓ should resolve SYNTH4 with glue
    ✓ should resolve SYNTH6 with glue

  10 passing (953ms)

@pinheadmz pinheadmz force-pushed the test-organize branch 2 times, most recently from b6b8bba to 2d70c4c Compare October 14, 2022 18:46
@pinheadmz pinheadmz changed the title test: organize Organize unit tests and add integration tests with hsd Oct 17, 2022
@pinheadmz pinheadmz force-pushed the test-organize branch 2 times, most recently from 8b1cd7f to 0c0795c Compare October 17, 2022 20:56
@pinheadmz pinheadmz force-pushed the test-organize branch 5 times, most recently from 12cb2b2 to d1640e6 Compare October 18, 2022 17:49
@pinheadmz pinheadmz mentioned this pull request Oct 19, 2022
@pinheadmz pinheadmz marked this pull request as ready for review October 19, 2022 14:58
Comment on lines +91 to +107
waitForSync() {
return new Promise((resolve, reject) => {
// Hack
setTimeout(() => {
resolve();
}, 5000);

// // TODO: Fix hnsd stdout parsing for chain height
// setTimeout(() => {
// reject(new Error('Timeout waiting for sync'));
// }, 5000);
// setInterval(() => {
// if (this.hnsdHeight === this.node.chain.height)
// resolve();
// }, 100);
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
waitForSync() {
return new Promise((resolve, reject) => {
// Hack
setTimeout(() => {
resolve();
}, 5000);
// // TODO: Fix hnsd stdout parsing for chain height
// setTimeout(() => {
// reject(new Error('Timeout waiting for sync'));
// }, 5000);
// setInterval(() => {
// if (this.hnsdHeight === this.node.chain.height)
// resolve();
// }, 100);
});
}
async waitForSync() {
if (!this.hnsd || !this.hnsd.killed) {
return new Promise(resolve => setTimeout(resolve, 100))
.then(() => this.waitForSync);
}
}

Comment on lines +40 to +53
this.hnsd.stdout.on('data', (data) => {
// TODO: `data` is always 8192 bytes and output gets cut off, why?
const chunk = data.toString('ascii');
const lines = chunk.split(/\n/);

for (const line of lines) {
const words = line.split(/\s+/);

if (words[0] !== 'chain' || words.length < 2)
continue;

this.hnsdHeight = parseInt(words[1].slice(1, -2));
}
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.hnsd.stdout.on('data', (data) => {
// TODO: `data` is always 8192 bytes and output gets cut off, why?
const chunk = data.toString('ascii');
const lines = chunk.split(/\n/);
for (const line of lines) {
const words = line.split(/\s+/);
if (words[0] !== 'chain' || words.length < 2)
continue;
this.hnsdHeight = parseInt(words[1].slice(1, -2));
}
});
this.hnsd.stdout.on('data', (data) => {
const chunk = data.toString('ascii');
// check for "chain (xxx)"
const height = chunk.match(/chain \((?<height>\d+)\).*(?![\s\S]*chain)/)?.groups?.height;
if (height)
this.hnsdHeight = parseInt(height);
if (timer)
clearTimeout(timer);
if (!this.hnsd.killed)
timer = setTimeout(() => {
this.hnsd.kill();
}, 200);
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a great-looking regex and cleans up the code. However, we (@nodech and myself) are finding that monitoring hnsd's stdout is not reliable. The waitForSync() mechanism will be improved in #102 where we can actually use DNS requests to ask hnsd for its chain height.

@pinheadmz
Copy link
Member Author

Update: added command make e2e in makefile so integration tests can be launched easily from root directory

@pinheadmz pinheadmz added this to the v2.0.0 milestone Oct 24, 2022
Copy link
Member

@rithvikvibhu rithvikvibhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool CI with 3 platforms! Both hnsd and integration tests work great locally.
I did forget to build for regtest (like the readme mentions) and the tests just timed out with no specific errors.

Can hnsd print the network name on start? It would help identify binaries and can also make the test suite confirm that it's build with the correct network. Just an idea, not blocking this PR.

Comment on lines +7 to +11
"dependencies": {
"bmocha": "^2.1.5",
"bsert": "0.0.10",
"hsd": "^4.0.1"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know bns is installed because of hsd, but since the test script explicitly uses bns, should be be included here?

@pinheadmz
Copy link
Member Author

Can hnsd print the network name on start? It would help identify binaries and can also make the test suite confirm that it's build with the correct network. Just an idea, not blocking this PR.

Yeah re-building for each network is annoying, maybe we can work that out in the future and start hnsd with a network argument. I think we need to avoid using hnsd's stdout for anything important, and using HS class won't really work either since the "API" ports will be different for each network.

I think the fastest solution to this may be a version command or something that could work like this:

$ hnsd --version
1.0.0 (regtest)

@pinheadmz
Copy link
Member Author

@rithvikvibhu added in 1715b09

--> make e2e
npm --prefix ./integration/ install

up to date, audited 37 packages in 332ms

found 0 vulnerabilities
npm --prefix ./integration/ run test

> [email protected] test
> bmocha --reporter spec test/*.js


  Basic sync & resolve
    1) "before all" hook

  0 passing (89ms)
  1 failing

  1) Basic sync & resolve
       "before all" hook:

      AssertionError [ERR_ASSERTION]: Network or version mismatch

      Expected values to be strictly equal:
      
      + actual - expected
      
      + '1.0.0 (main)\n'
      - '1.0.0 (regtest)\n'
                ^
      
      assert.strictEqual(
             ^

      at TestUtil.open (/Users/matthewzipkin/Desktop/work/hnsd/integration/test-util.js:36:12)
      at async Context.<anonymous> (/Users/matthewzipkin/Desktop/work/hnsd/integration/test/sync-resolve-test.js:18:5)

make: *** [e2e] Error 1

@rithvikvibhu
Copy link
Member

Sweet, version shows network and test fails. LGTM

@pinheadmz pinheadmz merged commit a4498e9 into handshake-org:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test Framework
3 participants