Skip to content

fix(tests): fix or ignore unstable tests#2365

Merged
shamardy merged 19 commits intodevfrom
ignore-unstable-tests
Feb 19, 2025
Merged

fix(tests): fix or ignore unstable tests#2365
shamardy merged 19 commits intodevfrom
ignore-unstable-tests

Conversation

@shamardy
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

I think simply ignoring the test might be a better option compared to commenting them out, compiler won't catch any changes that might affect the commented tests in the feature (if in any case)

@shamardy
Copy link
Copy Markdown
Collaborator Author

I think simply ignoring the test might be a better option compared to commenting them out, compiler won't catch any changes that might affect the commented tests in the feature (if in any case)

How do you ignore wasm_bindgen_test? this PR is still in progress either way as I will open issues for the ignored / commented out tests before making it ready for review.

@borngraced
Copy link
Copy Markdown

I think simply ignoring the test might be a better option compared to commenting them out, compiler won't catch any changes that might affect the commented tests in the feature (if in any case)

How do you ignore wasm_bindgen_test? this PR is still in progress either way as I will open issues for the ignored / commented out tests before making it ready for review.

apparently, we can't ignore wasm_bindgen tests..commenting is the only way I think.

@shamardy
Copy link
Copy Markdown
Collaborator Author

apparently, we can't ignore wasm_bindgen tests..commenting is the only way I think.

They are the only tests that are commented out for this reason :)

@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Feb 19, 2025

Maybe we can let the wasm_bindgen_test print something in the logs and we comment out the test body, what do you think @borngraced?

Testing this out with the latest commit

Edit:
I think this is better than nothing
https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/13413866520/job/37469861883#step:9:473
https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/13413866520/job/37469861883#step:9:512
https://github.com/KomodoPlatform/komodo-defi-framework/actions/runs/13413866520/job/37469861883#step:9:642

@shamardy shamardy force-pushed the ignore-unstable-tests branch from 85e435e to 79d59a7 Compare February 19, 2025 17:30
Comment on lines +266 to +284
// The purpose of this test is to verify that the `slurp_url` function successfully retrieves data from an external service.
// This retry loop was added to mitigate any test instability,
// since the target service (https://httpbin.org/get) can sometimes return a temporary failure (e.g., 502 Bad Gateway).
for _ in 0..6 {
let (status, headers, body) =
block_on(slurp_url("https://httpbin.org/get")).expect("Request failed unexpectedly");
if status.is_success() {
return;
}
log!(
"Request failed with status code: {}, headers: {:?}, body: {}, retrying...",
status,
headers,
String::from_utf8_lossy(&body)
);
block_on(Timer::sleep(5.));
}

panic!("Failed to retrieve data from the external service after multiple attempts. Please check the service status.");
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we can revert this change and depend on https://postman-echo.com/get uptime seems very okay

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done

@shamardy
Copy link
Copy Markdown
Collaborator Author

shamardy commented Feb 19, 2025

In this PR, the CI has been very stable since this commit eff9652. Any failures were due to a cancellation triggered by pushing a new commit. In total, about 11 commits have had their testing jobs successfully completed. There may still be occasional flaky tests in the future, but most should be resolved by re-running the failed jobs. If that doesn’t help, we’ll look for additional SPV servers or disable the problematic tests on the relevant machine, at least until a more permanent solution can be implemented.

Copy link
Copy Markdown

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

Great work!

@shamardy shamardy merged commit a559236 into dev Feb 19, 2025
@shamardy shamardy deleted the ignore-unstable-tests branch February 19, 2025 23:19

#[tokio::test]
#[cfg(not(windows))] // https://github.com/KomodoPlatform/atomicDEX-API/issues/1712
#[cfg(target_os = "linux")] // https://github.com/KomodoPlatform/atomicDEX-API/issues/1712
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why only linux OS instead of UNIX platform?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I guess it fails on MacOS too. Can we do "unix but not macos"?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, will look to fix it in another PR. Will add this to the nextest issue

@mariocynicys
Copy link
Copy Markdown
Collaborator

They are the only tests that are commented out for this reason :)

could we have a flakey feature flag and do cfg(flakey) around them instead of commenting? we can use that feature flag for other platforms also.

@shamardy
Copy link
Copy Markdown
Collaborator Author

could we have a flakey feature flag and do cfg(flakey) around them instead of commenting? we can use that feature flag for other platforms also.

Will open an issue for nextest and add this idea ref. https://nexte.st/docs/features/retries/#per-test-settings

dimxy pushed a commit to dimxy/komodo-defi-framework that referenced this pull request Feb 24, 2025
* dev: (24 commits)
  fix(eth-tpu): remove state from funding validation (GLEECBTC#2334)
  improvement(rpc-server): rpc server dynamic port allocation (GLEECBTC#2342)
  fix(tests): fix or ignore unstable tests (GLEECBTC#2365)
  fix(fs): make `filter_files_by_extension` return only files (GLEECBTC#2364)
  fix(derive_key_from_path): check length of current_key_material (GLEECBTC#2356)
  chore(release): bump mm2 version to 2.4.0-beta (GLEECBTC#2346)
  fix(tests): add additional testnet sepolia nodes to test code (GLEECBTC#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (GLEECBTC#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (GLEECBTC#2354)
  feat(tpu-v2): provide swap protocol versioning (GLEECBTC#2324)
  feat(wallet): add change mnemonic password rpc (GLEECBTC#2317)
  fix(tpu-v2): fix tpu-v2 wait for payment spend and extract secret (GLEECBTC#2261)
  feat(tendermint): unstaking/undelegation (GLEECBTC#2330)
  fix(utxo-withdraw): get hw ctx only when `PrivKeyPolicy` is trezor (GLEECBTC#2333)
  feat(event-streaming): API-driven subscription management (GLEECBTC#2172)
  fix(hash-types): remove panic, enforce fixed-size arrays (GLEECBTC#2279)
  fix(ARRR): store unconfirmed change output (GLEECBTC#2276)
  feat(tendermint): staking/delegation (GLEECBTC#2322)
  chore(deps): `timed-map` migration (GLEECBTC#2247)
  fix(mem-leak): `running_swap` never shrinks (GLEECBTC#2301)
  ...
dimxy pushed a commit that referenced this pull request Feb 26, 2025
* dev:
  feat(tendermint): claim delegation rewards (#2351)
  fix(eth-tpu): remove state from funding validation (#2334)
  improvement(rpc-server): rpc server dynamic port allocation (#2342)
  fix(tests): fix or ignore unstable tests (#2365)
  fix(fs): make `filter_files_by_extension` return only files (#2364)
dimxy pushed a commit that referenced this pull request Mar 5, 2025
* dev:
  feat(rpc): add is_success field to legacy MySwapStatusResponse (#2371)
  fix(key-derivation): use stored Argon2 parameters instead of default values (#2360)
  fix(tests): stabilize `tendermint_coin::test_claim_staking_rewards` (#2373)
  improvement(RPCs): group staking rpcs under a namespace (#2372)
  feat(tendermint): claim delegation rewards (#2351)
  fix(eth-tpu): remove state from funding validation (#2334)
  improvement(rpc-server): rpc server dynamic port allocation (#2342)
  fix(tests): fix or ignore unstable tests (#2365)
  fix(fs): make `filter_files_by_extension` return only files (#2364)
  fix(derive_key_from_path): check length of current_key_material (#2356)
  chore(release): bump mm2 version to 2.4.0-beta (#2346)
  fix(tests): add additional testnet sepolia nodes to test code (#2358)
  fix(swaps): maintain legacy compatibility for negotiation messages (#2353)
  refactor(SwapOps): impl defaults for protocol specific swapops fns (#2354)
  feat(tpu-v2): provide swap protocol versioning (#2324)
  feat(wallet): add change mnemonic password rpc (#2317)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants