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

[Session, Test] Session rollover test #332

Merged
merged 21 commits into from
Jan 29, 2024
Merged

[Session, Test] Session rollover test #332

merged 21 commits into from
Jan 29, 2024

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Jan 18, 2024

Summary

AI Summary

Summary generated by Reviewpad on 26 Jan 24 05:42 UTC

This pull request includes changes to the relayerproxy.go file in the testproxy package. It involves the following changes:

  • The function WithRelayerProxyDependencies has been renamed to WithRelayerProxyDependenciesForBlockHeight. It now takes an additional parameter blockHeight, which represents the block height returned by the block client's LastNBlock method.

  • A new function WithSuccessiveSessions has been introduced. This function creates sessions with session numbers ranging from 0 to sessionsCount-1 and adds them to the session map. These sessions are configured with the same service ID and application provided.

  • There is a TODO_TECHDEBT comment pertaining to suggested changes for the GetRelayResponseError function.

Issue

Type of change

Select one or more:

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Documentation
  • Other (specify)

Testing

  • Run all unit tests: make go_develop_and_test
  • Run E2E tests locally: make test_e2e
  • Run E2E tests on DevNet: Add the devnet-test-e2e label to the PR. This is VERY expensive, only do it after all the reviews are complete.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have performed a self-review of my own code
  • I have commented my code, updated documentation and left TODOs throughout the codebase

@red-0ne red-0ne added testing Test (or test utils) additions, fixes, improvements or other session Changes related to Session management labels Jan 18, 2024
@red-0ne red-0ne added this to the Shannon TestNet milestone Jan 18, 2024
@red-0ne red-0ne self-assigned this Jan 18, 2024
@red-0ne red-0ne changed the base branch from main to feat/session-rollover January 18, 2024 15:29
testutil/testproxy/relayerproxy.go Outdated Show resolved Hide resolved
testutil/testproxy/relayerproxy.go Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
@red-0ne red-0ne requested a review from Olshansk January 19, 2024 16:10
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

A couple minor NITS but shold be g2g right after.

@red-0ne Remember to update the base branch to main assuming you'll merge the other one in first. Please also trigger e2e tests after.

Comment on lines 82 to 83
"server1": {
ProxyName: "server1",
ProxyName: defaultServer,
Copy link
Member

Choose a reason for hiding this comment

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

This is an ultra NIT, but something I'm asking for just because I also always get confused between proxy/server/etc...

Do the key and proxyName need to be the same or can they be different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it's confusing.

The key and ProxyName have to be the same, as the config parser maps the proxy servers using their ProxyName as keys.

I was uncomfortable having a variable as a key but afterthought, I believe it will add more clarity. Let's having as

defaultProxyServer: {
  ProxyName: defaultProxyServer,
  ...
}

pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
pkg/relayer/proxy/proxy_test.go Outdated Show resolved Hide resolved
testutil/testproxy/relayerproxy.go Outdated Show resolved Hide resolved
@red-0ne red-0ne changed the base branch from feat/session-rollover to main January 26, 2024 05:34
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. If you just created a pull request, you might need to push another commit to produce a container image DevNet can utilize to spin up infrastructure. You can use make trigger_ci to push an empty commit.

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Jan 26, 2024
@red-0ne red-0ne merged commit a9686fd into main Jan 29, 2024
10 checks passed
@bryanchriswhite bryanchriswhite removed push-image CI related - pushes images to ghcr.io devnet-test-e2e labels May 16, 2024
@github-actions github-actions bot removed the devnet label May 16, 2024
okdas pushed a commit that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
session Changes related to Session management testing Test (or test utils) additions, fixes, improvements or other
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants