Skip to content

test: align constructor port expectations with Bitgesell - #10

Open
logical34 wants to merge 2 commits into
BitgesellOfficial:mainfrom
logical34:test-align-bitgesell-ports
Open

test: align constructor port expectations with Bitgesell#10
logical34 wants to merge 2 commits into
BitgesellOfficial:mainfrom
logical34:test-align-bitgesell-ports

Conversation

@logical34

Copy link
Copy Markdown

Description

This bounty/improvement PR aligns the constructor tests with the Bitgesell RPC defaults implemented by the client.

src/index.js defines the supported networks as:

  • mainnet: 8454
  • testnet: 18332

But test/client_test.js still had inherited Bitcoin Core expectations for the default/mainnet port (8332) and a regtest constructor case even though regtest is not in the client's supported network map. This PR updates the constructor expectations so the tests match the Bitgesell client behavior.

Verification

  • ./node_modules/.bin/eslint test/client_test.js
  • Direct constructor assertion smoke test:
    • new Client().port === 8454
    • new Client({ network: 'mainnet' }).port === 8454
    • new Client({ network: 'testnet' }).port === 18332
    • new Client({ network: 'regtest' }) throws Invalid network name
  • git diff --check

Bounty / payout

Submitted under the Bitgesell PR bounty/improvement program referenced from BitgesellOfficial/bitgesell#32 and BitgesellOfficial/bitgesell#39.

If approved, USDT/USDC EVM-compatible payout address:

0xd6634800410978b62D4a2317dCAF2871CDa9e1D3

Copilot AI review requested due to automatic review settings June 5, 2026 11:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the Client unit tests to reflect a new default mainnet port and removes the regtest default-port expectation.

Changes:

  • Update mainnet default port assertions from 8332 to 8454.
  • Remove the test asserting regtest defaults to port 18332.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread test/client_test.js
Comment on lines 68 to 74
it('should set default to port `18332` if network is `testnet`', () => {
should(new Client({ network: 'testnet' }).port).equal(18332);
});

it('should set default to port `18332` if network is `regtest`', () => {
should(new Client({ network: 'regtest' }).port).equal(18332);
});

it('should not have ssl enabled by default', () => {
should(new Client().ssl.enabled).equal(false);
should(new Client().ssl.strict).equal(false);
@MyTH-zyxeon

Copy link
Copy Markdown

Maintainer review-assist for the Bitgesell #81 improvement lane:

This looks like a clean one-file test-only update: the mainnet default port change to 8454 matches the Bitgesell network map described in the PR body, and testnet stays at 18332.

Before merge, I would make the removed regtest behavior explicit in checked-in coverage rather than leaving it only in the PR description:

  • Keep the new 8454 / 18332 constructor assertions.
  • Add a negative constructor test that new Client({ network: 'regtest' }) throws Invalid network name, matching the smoke-test claim in the PR body.
  • Name the case so it is clear regtest is intentionally unsupported by this client map, not accidentally untested.
  • Suggested verification: ./node_modules/.bin/eslint test/client_test.js, the targeted client constructor test, and git diff --check.

That keeps the #81 payout scope narrow: test expectation alignment only, with no live RPC calls, credentials, wallet actions, or network dependency.

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.

3 participants