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

fix: resolve node delete test failures #865

Closed
wants to merge 2 commits into from

Conversation

nathanklick
Copy link
Member

Description

This pull request changes the following:

  • Changes the _getNodeClient method to use the second node only by default.

Related Issues

@nathanklick nathanklick self-assigned this Nov 22, 2024
@nathanklick nathanklick added Bug A error that causes the feature to behave differently than what was expected based on design docs P0 An issue impacting production environments or impacting multiple releases or multiple individuals. labels Nov 22, 2024
@nathanklick nathanklick marked this pull request as ready for review November 22, 2024 17:45
@nathanklick nathanklick requested review from leninmehedy and a team as code owners November 22, 2024 17:45
Copy link
Contributor

github-actions bot commented Nov 22, 2024

Unit Test Results - Linux

  1 files  ±0   36 suites  ±0   4s ⏱️ -1s
109 tests ±0  109 ✅ ±0  0 💤 ±0  0 ❌ ±0 
118 runs  ±0  118 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit cc48bf1. ± Comparison against base commit 1d9452e.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 22, 2024

Unit Test Results - Windows

  1 files  ±0   36 suites  ±0   13s ⏱️ ±0s
109 tests ±0  109 ✅ ±0  0 💤 ±0  0 ❌ ±0 
118 runs  ±0  118 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit cc48bf1. ± Comparison against base commit 1d9452e.

♻️ This comment has been updated with latest results.

@@ -214,18 +214,20 @@ export class AccountManager {
* @returns a node client that can be used to call transactions
*/
async _getNodeClient (namespace: string, networkNodeServicesMap: Map<string, NetworkNodeServices>, operatorId: string,
operatorKey: string, useFirstNodeOnly = true) {
operatorKey: string, useSecondNodeOnly = true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if they do a solo node delete but then delete the second node?

Copy link
Contributor

@jeromy-cannon jeromy-cannon Nov 22, 2024

Choose a reason for hiding this comment

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

also, I'm running locally with the new lower docker CPU and RAM settings, and the delete is failing before it gets to the delete subcommand

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a terrible trade off at the moment based on how and when this client is instantiated. Right now all the tests only delete the first node.

Copy link
Contributor

Choose a reason for hiding this comment

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

so you added code to help a test case pass?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a hacky fix until we untangle some other issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't it have been easier to just change the test case to not delete node1?

Copy link
Member Author

Choose a reason for hiding this comment

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

so you added code to help a test case pass?

No not exactly, it's also to provide some relief due to SDK Client issues.

@nathanklick nathanklick force-pushed the 00864-Fix-Node-Delete-Tests branch from 37468ae to cc48bf1 Compare November 22, 2024 21:48
@nathanklick nathanklick deleted the 00864-Fix-Node-Delete-Tests branch November 22, 2024 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A error that causes the feature to behave differently than what was expected based on design docs P0 An issue impacting production environments or impacting multiple releases or multiple individuals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resolve issues with Node Delete E2E tests
2 participants