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

not very basic maintenance #1004

Merged
merged 15 commits into from
Jun 5, 2024
Merged

Conversation

faddat
Copy link
Contributor

@faddat faddat commented May 7, 2024

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

Trouble is that tests would fail at random.

  • Decided to upgrade interchaintest
  • Interchaintest's changes required changes to Juno's interchain tests

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

@faddat
Copy link
Contributor Author

faddat commented May 13, 2024

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

@JakeHartnell
Copy link
Contributor

Looks good, any thoughts on the failing test?

@faddat
Copy link
Contributor Author

faddat commented May 13, 2024

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
Copy link
Contributor Author

faddat commented May 14, 2024

@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
Copy link
Contributor Author

faddat commented May 14, 2024

oops this time it was feepay

might as well add another godoc

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

oh snap now it is ibchooks

let's do a 4th

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

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
Copy link
Contributor Author

faddat commented May 14, 2024

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
Copy link
Contributor Author

faddat commented May 14, 2024

Ah, pfm doesn't work again

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

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
Copy link
Contributor Author

faddat commented May 14, 2024

Clock again

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

"but sometimes clock doesn't poop the bed"

Yes

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

Pfm too

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

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
Copy link
Contributor Author

faddat commented May 14, 2024

oop, ictest-basic this time

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

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
Copy link
Contributor Author

faddat commented May 14, 2024

oops tests need to be rewritten

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

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
Copy link
Contributor Author

faddat commented May 14, 2024

@faddat
Copy link
Contributor Author

faddat commented May 14, 2024

@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 faddat changed the title basic maintenance not very basic maintenance May 14, 2024
@faddat
Copy link
Contributor Author

faddat commented May 15, 2024

  • ictest-basic
  • ictest-globalfee
  • ictest-upgrade

@faddat
Copy link
Contributor Author

faddat commented May 15, 2024

@JakeHartnell - wrt this pr and that test:

  • welp, interchaintest did not work to begin with
  • then I updated it
  • then the tests needed to be updated
  • now idk if I broke stuff or not

Maybe just figure that:

  • the merge of this PR signals the release of juno v23
  • v24 is sdk 50

@faddat
Copy link
Contributor Author

faddat commented May 18, 2024

@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
Copy link
Contributor Author

faddat commented May 18, 2024

Don't pass in CI

  • basic
  • pfm
  • cwhooks

@faddat
Copy link
Contributor Author

faddat commented May 18, 2024

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

@faddat
Copy link
Contributor Author

faddat commented May 19, 2024

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
Copy link
Member

dimiandre commented Jun 4, 2024

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!

@JakeHartnell JakeHartnell merged commit 175333d into CosmosContracts:main Jun 5, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants