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

feat: parallel testing #2821

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
7 changes: 1 addition & 6 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ jobs:
id: start-local-node
if: ${{ steps.build-sdk.conclusion == 'success' && !cancelled() && always() }}
run: |
${{ env.CG_EXEC }} npx @hashgraph/hedera-local start -d -—network local --balance=100000 --network-tag=0.57.0
${{ env.CG_EXEC }} npx @hashgraph/hedera-local start -d -—network local --balance=10000000 --network-tag=0.57.0
# Wait for the network to fully start
sleep 30

Expand All @@ -143,11 +143,6 @@ jobs:
if: ${{ steps.build-sdk.conclusion == 'success' && steps.stop-local-node.conclusion == 'success' && !cancelled() && always() }}
run: ${{ env.CG_EXEC }} task build

- name: Unit Test @hashgraph/cryptography
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are the unit tests removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We run them twice, the second time is when we run the codecov tests so I don't see a reason to run them twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

The removed unit tests are running inside the packages/cryptography folder, which is different then the next unit tests. If you are completely sure that removing the unit tests here is ok I will resolve the conversation, but please take another look and then confirm.

working-directory: packages/cryptography
if: ${{ steps.build-sdk.conclusion == 'success' && steps.stop-local-node.conclusion == 'success' && !cancelled() && always() }}
run: ${{ env.CG_EXEC }} task test:unit

- name: Codecov @hashgraph/cryptography
working-directory: packages/cryptography
if: ${{ steps.build-sdk.conclusion == 'success' && steps.stop-local-node.conclusion == 'success' && !cancelled() && always() }}
Expand Down
4 changes: 2 additions & 2 deletions Taskfile.yml
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,11 @@ tasks:

"test:integration:node":
cmds:
- npx mocha --exit -r @babel/register -r chai/register-expect.js "test/integration/*.js" --timeout 120000 {{.CLI_ARGS}}
- npx mocha --exit --parallel -jobs 4 -r @babel/register -r chai/register-expect.js "test/integration/*.js" --timeout 120000 {{.CLI_ARGS}}

"test:integration:codecov":
cmds:
- npx c8 --reporter=lcov --reporter=text-summary ./node_modules/.bin/mocha --exit -r @babel/register -r chai/register-expect.js "test/integration/*.js" --timeout 120000 {{.CLI_ARGS}}
- npx c8 --reporter=lcov --reporter=text-summary ./node_modules/.bin/mocha --exit --parallel -jobs 4 -r @babel/register -r chai/register-expect.js "test/integration/*.js" --timeout 120000 {{.CLI_ARGS}}
- npx c8 report

"update:proto":
Expand Down
31 changes: 21 additions & 10 deletions src/Executable.js
Original file line number Diff line number Diff line change
Expand Up @@ -523,6 +523,10 @@ export default class Executable {
* @returns {Promise<OutputT>}
*/
async execute(client, requestTimeout) {
// we check if its local node then backoff mechanism should be disabled
// and we increase the retry attempts
const isLocalNode = client.network["127.0.0.1:50211"] != null;

// If the logger on the request is not set, use the logger in client
// (if set, otherwise do not use logger)
this._logger =
Expand Down Expand Up @@ -555,22 +559,23 @@ export default class Executable {
this._minBackoff = client.minBackoff;
}

// If the max attempts on the request is not set, use the default value in client
// If the default value in client is not set, use a default of 10.
//
// FIXME: current implementation is wrong, update to follow comment above.
const maxAttempts =
client._maxAttempts != null
? client._maxAttempts
: this._maxAttempts;

// Save the start time to be used later with request timeout
const startTime = Date.now();

// Saves each error we get so when we err due to max attempts exceeded we'll have
// the last error that was returned by the consensus node
let persistentError = null;

// If the max attempts on the request is not set, use the default value in client
// If the default value in client is not set, use a default of 10.
//
// FIXME: current implementation is wrong, update to follow comment above.
// ... existing code ...
const LOCAL_NODE_ATTEMPTS = 1000;
const maxAttempts = isLocalNode
? LOCAL_NODE_ATTEMPTS
: (client._maxAttempts ?? this._maxAttempts);

// Checks if has a valid nodes to which the TX can be sent
if (this.transactionNodeIds.length) {
const nodeAccountIds = this._nodeAccountIds.list.map((nodeId) =>
Expand Down Expand Up @@ -773,6 +778,7 @@ export default class Executable {
switch (shouldRetry) {
case ExecutionState.Retry:
await delayForAttempt(
isLocalNode,
attempt,
this._minBackoff,
this._maxBackoff,
Expand Down Expand Up @@ -840,12 +846,17 @@ export default class Executable {
/**
* A simple function that returns a promise timeout for a specific period of time
*
* @param {boolean} isLocalNode
* @param {number} attempt
* @param {number} minBackoff
* @param {number} maxBackoff
* @returns {Promise<void>}
*/
function delayForAttempt(attempt, minBackoff, maxBackoff) {
function delayForAttempt(isLocalNode, attempt, minBackoff, maxBackoff) {
if (isLocalNode) {
return new Promise((resolve) => setTimeout(resolve, minBackoff));
}

// 0.1s, 0.2s, 0.4s, 0.8s, ...
const ms = Math.min(
Math.floor(minBackoff * Math.pow(2, attempt)),
Expand Down
47 changes: 22 additions & 25 deletions src/query/Query.js
Original file line number Diff line number Diff line change
Expand Up @@ -278,35 +278,32 @@ export default class Query extends Executable {
this._operator =
this._operator != null ? this._operator : client._operator;

// If the payment transaction ID is not set
if (this._paymentTransactionId == null) {
// And payment is required
if (this._isPaymentRequired()) {
// Assign the account IDs to which the transaction should be sent.
this.transactionNodeIds = Object.values(client.network).map(
(accountNodeId) => accountNodeId.toString(),
);
// And payment is required
if (this._isPaymentRequired()) {
// Assign the account IDs to which the transaction should be sent.
this.transactionNodeIds = Object.values(client.network).map(
(accountNodeId) => accountNodeId.toString(),
);

// And the client has an operator
if (this._operator != null) {
// Generate the payment transaction ID
this._paymentTransactionId = TransactionId.generate(
this._operator.accountId,
);
} else {
// If payment is required, but an operator did not exist, throw an error
throw new Error(
"`client` must have an `operator` or an explicit payment transaction must be provided",
);
}
} else {
// If the payment transaction ID is not set, but this query doesn't require a payment
// set the payment transaction ID to an empty transaction ID.
// FIXME: Should use `TransactionId.withValidStart()` instead
// And the client has an operator
if (this._operator != null) {
// Generate the payment transaction ID
this._paymentTransactionId = TransactionId.generate(
new AccountId(0),
this._operator.accountId,
);
} else {
// If payment is required, but an operator did not exist, throw an error
throw new Error(
"`client` must have an `operator` or an explicit payment transaction must be provided",
);
}
} else {
// If the payment transaction ID is not set, but this query doesn't require a payment
// set the payment transaction ID to an empty transaction ID.
// FIXME: Should use `TransactionId.withValidStart()` instead
this._paymentTransactionId = TransactionId.generate(
new AccountId(0),
);
}

let cost = new Hbar(0);
Expand Down
Loading