fix(tests): fix failing tests #2085
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mariocynicys
force-pushed
the
fix-failing-tests
branch
2 times, most recently
from
3210807
to
7c83003
Compare
|
@mariocynicys please don't forget to remove |
mariocynicys
force-pushed
the
fix-failing-tests
branch
from
728e3d7
to
4b42f2f
Compare
laruh
reviewed
Mar 14, 2024
There was a problem hiding this comment.
Thanks for the fixes!
I have just a note for now.
Please add dependency changes info in PR description (when PR is r2r, or now if you wana). What was deleted/added/version changes and the reason of it.
Something like this:
Deps changes:
Needed to upgrade version of cosmrs due to breaking changes in tendermint-rpc which fail our tests.
cosmrs = { version = "0.7", default-features = false } -> cosmrs = { version = "0.14.0", default-features = false }
etc.
We have established this rule for PRs to simplify the security team's process of conducting code security checks.
|
PS: branch started to have conflicts |
mariocynicys
force-pushed
the
fix-failing-tests
branch
3 times, most recently
from
bc20355
to
a74eb1c
Compare
Deps changes:Deps updated (coins):
Deps removed (coins):
Deps updated (mm2_net):
CI has been updated to install The latest tendermint version is 0.34 (cosmrs 0.15) but we couldn't use it since they updated their This could have be solved by depending on different versions of prost at the same time (cosmrs & tendermint on prost 0.12 while tonic on prost 0.11) but this is not really feasible since prost's derive macro |
mariocynicys
force-pushed
the
fix-failing-tests
branch
2 times, most recently
from
0b99ca9
to
d6f127a
Compare
|
@mariocynicys please push |
mariocynicys
force-pushed
the
fix-failing-tests
branch
3 times, most recently
from
7415507
to
13bf572
Compare
These are logs of flaky tests since github will purge these logs soon:
mm2_tests::mm2_tests_inner::set_price_with_cancel_previous_should_broadcast_cancelled_message (windows)Other tests ( qrc20::qrc20_tests::test_send_taker_feecustom_futures::repeatable::tests::test_until_successTest mm2_tests::best_orders_tests::best_orders_must_return_duplicate_for_orderbook_tickers |
borngraced
requested changes
Mar 25, 2024
| @@ -2431,6 +2437,7 @@ async fn sign_transaction_with_keypair( | |||
| Ok(( | |||
| tx.sign(key_pair.secret(), coin.chain_id), | |||
| web3_instances_with_latest_nonce, | |||
| nonce_lock, | |||
| )) | |||
| } | |||
There was a problem hiding this comment.
what difference does it make if we just drop the nonce_lock before returning from this function and returning it without using e.g
async fn sign_transaction_with_keypair(
ctx: MmArc,
coin: &EthCoin,
key_pair: &KeyPair,
value: U256,
action: Action,
data: Vec<u8>,
gas: U256,
) -> Result<(SignedEthTx, Vec<Web3Instance>), TransactionErr> {
let mut status = ctx.log.status_handle();
macro_rules! tags {
() => {
&[&"sign"]
};
}
let nonce_lock = coin.nonce_lock.lock().await;
status.status(tags!(), "get_addr_nonce…");
let (nonce, web3_instances_with_latest_nonce) =
try_tx_s!(coin.clone().get_addr_nonce(coin.my_address).compat().await);
status.status(tags!(), "get_gas_price…");
let gas_price = try_tx_s!(coin.get_gas_price().compat().await);
let tx = UnSignedEthTx {
nonce,
gas_price,
gas,
action,
value,
data,
};
let _ = nonce_lock;
Ok((
tx.sign(key_pair.secret(), coin.chain_id),
web3_instances_with_latest_nonce,
))
}There was a problem hiding this comment.
A nonce can not be reused in another tx. We get the nonce from RPC nodes (we pick the highest of them) and use it in our tx.
If another tx is being created at the same time from another thread (which is the case tested by test_nonce_lock test) they would get the same nonce from the RPC. And the eth network will reject the second one.
We will want to return the lock so the caller can have the nonce locked until the transaction is relayed successfully and the nonce is updated on the connected RPC nodes.
There was a problem hiding this comment.
ok thanks for the explanation 🙏🏼
onur-ozkan
reviewed
Mar 25, 2024
| @@ -173,5 +172,5 @@ mm2_test_helpers = { path = "../mm2_test_helpers" } | |||
| wagyu-zcash-parameters = { version = "0.2" } | |||
|
|
|||
| [build-dependencies] | |||
| prost-build = { version = "0.10.4", default-features = false } | |||
| tonic-build = { version = "0.7", default-features = false, features = ["prost", "compression"] } | |||
| prost-build = { version = "0.11", default-features = false } | |||
There was a problem hiding this comment.
Can you bump this one to the same version too?
There was a problem hiding this comment.
oh thanks, that would hopefully help reduce duplications.
Cargo.lock
Outdated
There was a problem hiding this comment.
Most of these are duplicated dependencies in our dependency stack. Can we somehow manage to reduce this list? This will increase the build time.
There was a problem hiding this comment.
I think we can upgrade other deps to achieve that. I will give it a try.
p.s. is there some tool that analyzes such a toml & lock files and advises what versions to use to reduce duplication?
There was a problem hiding this comment.
https://github.com/EmbarkStudios/cargo-deny can help to detect duplications but I don't think there is tool that says "bump these to reduce duplication".
I tried it separately a 100 times on macos locally and it never fails, maybe we should use unique tickers for this test to have a unique orderbook entry for it in case other concurrent tests affect it. If that doesn't solve it, maybe increase the pause a bit But I don't think increasing the pause will fix it. |
This is probably due to using the same private key across CI machines komodo-defi-framework/mm2src/coins/qrc20/qrc20_tests.rs Lines 316 to 319 in 7a8770c We should probably move it to qrc20 docker tests to use a different private key for each run. |
It's ok to increase the |
I see that you already fixed this test by adding funds to the address https://mempool.space/testnet/tx/faca07c3ed403d57b54e54b373439480be483dbbec6604e8b81dd8a9f246ceec I guess it doesn't fail anymore. |
|
@mariocynicys |
onur-ozkan
reviewed
Mar 26, 2024
.github/workflows/dev-build.yml
Outdated
Comment on lines
168
to
176
| env: | ||
| AVAILABLE: ${{ secrets.FILE_SERVER_KEY }} | ||
| if: ${{ env.AVAILABLE != '' }} | ||
| if: env.FILE_SERVER_AVAILABLE |
There was a problem hiding this comment.
Did you test this? The last time I tried it wasn't working or supported directly like this.
There was a problem hiding this comment.
Nope, I thought this should be intuitive, but truly nothing is intuitive about github actions 😢.
The case you want to make sure of here is that when this is run from another repo it shouldn't fail here?
There was a problem hiding this comment.
There was a problem hiding this comment.
In that case we have to revert those
.github/workflows/fmt-and-lint.yml
Outdated
| @@ -7,34 +7,38 @@ concurrency: | |||
|
|
|||
| jobs: | |||
| fmt-and-lint: | |||
| name: Style Checks | |||
There was a problem hiding this comment.
this title isn't correct, we lint the code from many aspects which isn't all about styles
Suggested change
| name: Style Checks | |
| name: X86 Format and Lint Check |
There was a problem hiding this comment.
I think we might wanna omit and X86 part tho. and also have wasm with the same name.
I think this makes the workflow looks more organized:
There was a problem hiding this comment.
Problem is we do have cross compilation pipelines (armv7, aarch64, x86, wasm, etc.) that's why I prefer explicitly adding targets in the pipeline names.
.github/workflows/fmt-and-lint.yml
Outdated
| run: cargo clippy --all-targets --all-features -- --D warnings | ||
|
|
||
| wasm-lint: | ||
| name: Style Checks (wasm) |
There was a problem hiding this comment.
Suggested change
| name: Style Checks (wasm) | |
| name: WASM Lint Check |
Yup that was my guess. Cool, will move this one to docker then.
Oh yeah, I totally forgot I did that xD.
I will. My experience though is that it fails all the time but sami had it working sometimes locally. The test doesn't seem all that complicated which makes me think it is a network issue. |
the testnet ibc channels are unmaintained and get expired a lot, making these specific two tendermint tests fail also fix some formatting issue
looks like secrets can't be probagated to the global env of a workflow
This reverts commit 375a93d.
not all dups of these deps are cleared. e.g. libp2p uses futures-rustls 0.22, tokio-tungstenite-wasm uses tokio-tungstenite which pulls an old version of tokio-rustls.
+ remove ignored wasm test squashed: fix quik -> qick
they used eth so moved them to docker tests
…aking it an opiton field
mariocynicys
force-pushed
the
fix-failing-tests
branch
from
63df0c1
to
9577388
Compare
two cases for print! were ommited but they seem used in an interactive test to prompt the user for a password
…t available yet and also use the trade_base_rel function from the docker commons
looks like using IP with ssl encryption isn't a common practice [1], also tls connector from tokio-rustls explicilty calls the ServerName argument in `connect` method `domain`, so lets stick with that. https://stackoverflow.com/questions/2043617/is-it-possible-to-have-ssl-certificate-for-ip-address-not-domain-name
mariocynicys
force-pushed
the
fix-failing-tests
branch
from
8e814de
to
dd2c7b3
Compare
|
Will merge this now and continue review after merging since this is needed in #2079 |
dimxy
added a commit
to dimxy/komodo-defi-framework
that referenced
this pull request
Apr 16, 2024
* dev: fix(tests): fix failing tests (KomodoPlatform#2085) fix(wasm): websocket url validation (KomodoPlatform#2096) deps(zcoin): use librustzcash that uses the same `aes` version as mm2 (KomodoPlatform#2095)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes failing unit and integration tests.
The tests were failing for various reasons:
test_nonce_lock)didn't have a fund provider on MacOs and Windows (there is the possibility this might flaky instead, will be known from the logs of multiple CI runs when they are available)has a flaky nonce locking (doesn't work all the time).test_process_price_request) used an outdated URL.index: boolwhich we fail to parse since we usetendermint-rpc = 23.7(old tendermint version). was also because some low balance test addresses and failing IBC channels.JSTtoken onETHcan't be enabled (contract address not found), probably because the used ETH_DEV_NODE changed or something. Should be updated to use a local geth node instead.Deps changes:
Deps updated (coins):
tendermint-rpc = { version = "=0.23.7", default-features = false }->tendermint-rpc = { version = "0.32.0", default-features = false }cosmrs = { version = "0.7", default-features = false }->cosmrs = { version = "0.14.0", default-features = false }prost = { version = "0.10" }->prost = { version = "0.11" }tonic = { version = "0.7", default-features = false, features = ["prost", "codegen", "compression"] }->tonic = { version = "0.9", default-features = false, features = ["prost", "codegen", "gzip"] }prost-build = { version = "0.10.4", default-features = false }->prost-build = { version = "0.11", default-features = false }tonic-build = { version = "0.7", default-features = false, features = ["prost", "compression"] }->tonic-build = { version = "0.9", default-features = false, features = ["prost"] }Deps removed (coins):
tendermint-config = { version = "0.23.7", default-features = false }- didn't seem to be usedDeps updated (mm2_net):
prost = { version = "0.10" }->prost = { version = "0.11" }tonic = { version = "0.7", default-features = false, features = ["prost", "codegen"] }->tonic = { version = "0.9", default-features = false, features = ["prost", "codegen"] }#2085 (comment)