CosmosContracts / juno

Open Source Platform for Interoperable Smart Contracts
Apache License 2.0
311 stars 171 forks source link

not very basic maintenance #1004

Closed faddat closed 4 months ago

faddat commented 5 months ago

This PR originally set out to do basic maintenance on Juno.

Trouble is that tests would fail at random.

....so this PR upgrades numerous libraries used in Juno, as well as its test suite.

faddat commented 4 months ago

I think that that e2e is just flaky, you probably need to rerun CI in order for this to pass.

JakeHartnell commented 4 months ago

Looks good, any thoughts on the failing test?

faddat commented 4 months ago

just should need to click the button that runs it again.

It is likely a "flaky test" -- these are common with interchaintest, which doesn't generally pass its own test suite due to flakiness

faddat commented 4 months ago

@JakeHartnell good example of flaky tests: this didn't pass, again. But it didn't pass on different tests.

Please note that in the first run, pfm failed.

In the second run, clock and upgrade failed.

Why?

ictest isn't consistent

Let's try again for fun

faddat commented 4 months ago

oops this time it was feepay

might as well add another godoc

faddat commented 4 months ago

oh snap now it is ibchooks

let's do a 4th

faddat commented 4 months ago

Looks good, any thoughts on the failing test?

If we follow the same approach as is used in the ibctest upstream, and keep spamming with commits that change nothing, it should eventually pass.

Of course it means that all of the failures were meaningless....

or were they?

faddat commented 4 months ago
Screenshot 2024-05-14 at 8 54 54 AM

Look familiar?

It's the same happening in the interchaintest repo when it tries to run their own tests

faddat commented 4 months ago

Ah, pfm doesn't work again

faddat commented 4 months ago

Remember changes to comments do not actually change what the code does, what you're seeing is the level of flakiness in the IC tests

faddat commented 4 months ago

Clock again

faddat commented 4 months ago

"but sometimes clock doesn't poop the bed"

Yes

faddat commented 4 months ago

Pfm too

faddat commented 4 months ago

good news is that it is probably not the modules themselves, just the code that tests them

doing a 6th test run

...btw probably the way they passed in the past is whomever was maintaining just clicked the button to run them till they all passed

Don't really know another way actually

faddat commented 4 months ago

oop, ictest-basic this time

faddat commented 4 months ago

go.mod is much cleaner now anyhow, and comet is the same version on the chain and in the tests, and a few other libs as a result of that.

faddat commented 4 months ago

oops tests need to be rewritten

faddat commented 4 months ago
go: downloading modernc.org/memory v1.6.0
go: downloading github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec
# github.com/CosmosContracts/juno/tests/interchaintest/helpers
Error: helpers/gov.go:41:60: invalid operation: height + searchHeightDelta (mismatched types int64 and uint64)
Error: helpers/gov.go:41:86: cannot use proposalID (variable of type string) as int64 value in argument to cosmos.PollForProposalStatus
Error: helpers/gov.go:41:105: undefined: cosmos.ProposalStatusPassed
FAIL    github.com/CosmosContracts/juno/tests/interchaintest [build failed]
FAIL
make: *** [Makefile:170: ictest-feepay] Error 1
Error: Process completed with exit code 2.
faddat commented 4 months ago
faddat commented 4 months ago

@JakeHartnell tl;dr here:

1) interchaintest is pretty unstable and will throw unanticipated results 2) juno's test code wasn't kept up to date with strangelove's interchaintest code, which was changed to follow the cosmos-sdk conventions around feb 20 3) juno's test code is now updated 4) that might not really matter for the stability of the tests 5) you should have a "run failed test again" option as a repo admin. The way tests passed previously is that it was clicked many times until all of the tests passed 6) or maybe I made type errors in the transition

....so that's what is up with the failed test

BTW if you'd like to put proof behind the above assertions, just recreate the scenario of any recent pull request.

If the check mark isn't green right away just click that magic button a few times

Since we are working with tests it is actually important to prove this kinda thing.

Unfortunately.

Also from looking at some of the logs, it could be that there's code baked into the image server at SL that we don't have but now that needs to be updated, to be quite honest I'm not really sure at this point.

I just don't know of anywhere where ICT works in a consistent manner.

faddat commented 4 months ago
faddat commented 4 months ago

@JakeHartnell - wrt this pr and that test:

Maybe just figure that:

faddat commented 4 months ago

@JakeHartnell I have no idea if these work or ever worked better than clicking the button for em 20 times. I will try and check some more.

faddat commented 4 months ago

Don't pass in CI

faddat commented 4 months ago

cwhooks passes locally but not in ci pfm passes locally but not in ci

faddat commented 4 months ago

this is good to go, guess need to either add a delay to the cwhooks test (something with firing up the image) or just run it locally.

dimiandre commented 4 months ago

ci passed now.

instead of re-run all tests, re running only failed ones gives an higher success rate

we should migrate away from ict

thanks for handling this!