-
Notifications
You must be signed in to change notification settings - Fork 12
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
[Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" #253
Conversation
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. |
Did a replace of this: logger\.(Info|Error)\("([^"]+)"(?:, (.*))?\) With: logger.$1(fmt.Sprintf("$2", $3))
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. |
@okdas Seems like the change I made from |
The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks. |
@bryanchriswhite @h5law Please see my comment above regarding imports. I've re-validated that @okdas Can you please verify this change. I believe this was an artifact that's no longer necessary (@red-0ne and I discovered it yesterday) but would appreciate another set of 👀 . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM few files to goimports-reviser
left but approving so we can merge <3
x/application/keeper/msg_server_undelegate_from_gateway_test.go
Outdated
Show resolved
Hide resolved
…eviser -rm-unused -set-alias -format -output write
BUMP: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a couple of NITs but otherwise let's 🚢
// NB: We are avoiding the use of require.Len here because it provides unreadable output | ||
// TODO_TECHDEBT: Due to the speed of the blocks of the LocalNet sequencer, along with the small number | ||
// of blocks per session, multiple claims may be created throughout the duration of the test. Until | ||
// these values are appropriately adjusted | ||
require.Greater(s, len(allClaimsRes.Claim), len(preExistingClaims), "number of claims must have increased") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did experiment with Lenf, but I did experiment with giving require.Len an unformatted string.
I'm assuming one of those is suppose to be a "didn't". 😅
For the record, in other cases, I would argue that #Lenf()
produces the least ambiguous output. I agree that the output can be large if the list is not empty; however, that information is useful when debugging, and isn't obscuring the reason for failure as it's on its own line.
#Lenf()
func TestSanity(t *testing.T) {
x := []struct{ a, b int }{{1, 2}, {3, 4}}
require.Lenf(t, x, 3, "x should have 3 elements, got %d", len(x))
}
#Len()
func TestSanity(t *testing.T) {
x := []struct{ a, b int }{{1, 2}, {3, 4}}
require.Len(t, x, 3)
}
#Greater()
func TestSanity(t *testing.T) {
x := []struct{ a, b int }{{1, 2}, {3, 4}}
require.Greater(t, len(x), 2)
}
commit df73cfa Author: Bryan White <[email protected]> Date: Thu Dec 14 01:06:03 2023 +0100 refactor: `NewMinedRelay` to shared testutil (#262) commit 410965a Author: Bryan White <[email protected]> Date: Thu Dec 14 01:05:52 2023 +0100 fix: PR template typo 2 (#269) commit dd7df68 Author: Daniel Olshansky <[email protected]> Date: Wed Dec 13 13:45:26 2023 -0800 [Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" (#253) This is aiming to fix multiple issues in E2E tests and just general QoL improvements: 1. Adding a comment in `relay.feature` about the need to run `make supplier1_stake && make app1_stake` until we fix #180 (known issue) 2. Replacing the reference to host `sequencer-poktroll-sequencer` with `localhost` (see first screenshot below) 3. Fixed a bug leading us to assert against a `MsgSubmitProof` event instead of a `MsgCreateClaim` event (see second screenshot below) 4. Fixed on-chain logging under `x/*` by replacing `logger\.(Info|Error)\("([^"]+)"(?:, (.*))?\)` with `logger.$1(fmt.Sprintf("$2", $3))` so it works as expected 5. Some quality-of-life comments & TODOs 6. Minor additions to unit tests from #220 commit d621631 Author: Redouane Lakrache <[email protected]> Date: Wed Dec 13 16:24:34 2023 +0100 [SDK] feat: Have distinct JSON-RPC and gRPC urls (#261) * feat: Have distinct JSON-RPC and gRPC urls * chore: Trigger e2e tests * chore: Fix import groups commit 4e30e27 Author: Bryan White <[email protected]> Date: Wed Dec 13 14:28:36 2023 +0100 fix PR template testing checklist item (#268)
…#253) This is aiming to fix multiple issues in E2E tests and just general QoL improvements: 1. Adding a comment in `relay.feature` about the need to run `make supplier1_stake && make app1_stake` until we fix #180 (known issue) 2. Replacing the reference to host `sequencer-poktroll-sequencer` with `localhost` (see first screenshot below) 3. Fixed a bug leading us to assert against a `MsgSubmitProof` event instead of a `MsgCreateClaim` event (see second screenshot below) 4. Fixed on-chain logging under `x/*` by replacing `logger\.(Info|Error)\("([^"]+)"(?:, (.*))?\)` with `logger.$1(fmt.Sprintf("$2", $3))` so it works as expected 5. Some quality-of-life comments & TODOs 6. Minor additions to unit tests from #220
…im-proof * pokt/main: Fix bug introduced by #252 where genesis file was no longer being copied to the right location [Docs] Introduce Docusaurus documentation (#252) [Cleanup] Centralzie websocket url -> endpoint changes (#272) refactor: `NewMinedRelay` to shared testutil (#262) fix: PR template typo 2 (#269) [Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" (#253) [SDK] feat: Have distinct JSON-RPC and gRPC urls (#261)
…or/supplier-keys * issues/141/refactor/claim-proof: chore: review feedback improvements chore: review feedback improvements Fix bug introduced by #252 where genesis file was no longer being copied to the right location [Docs] Introduce Docusaurus documentation (#252) [Cleanup] Centralzie websocket url -> endpoint changes (#272) refactor: `NewMinedRelay` to shared testutil (#262) fix: PR template typo 2 (#269) [Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" (#253) [SDK] feat: Have distinct JSON-RPC and gRPC urls (#261)
…ctor/supplier-errors * issues/141/refactor/supplier-keys: chore: review feedback improvements chore: review feedback improvements Fix bug introduced by #252 where genesis file was no longer being copied to the right location [Docs] Introduce Docusaurus documentation (#252) [Cleanup] Centralzie websocket url -> endpoint changes (#272) refactor: `NewMinedRelay` to shared testutil (#262) fix: PR template typo 2 (#269) [Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" (#253) [SDK] feat: Have distinct JSON-RPC and gRPC urls (#261)
…#253) This is aiming to fix multiple issues in E2E tests and just general QoL improvements: 1. Adding a comment in `relay.feature` about the need to run `make supplier1_stake && make app1_stake` until we fix #180 (known issue) 2. Replacing the reference to host `sequencer-poktroll-sequencer` with `localhost` (see first screenshot below) 3. Fixed a bug leading us to assert against a `MsgSubmitProof` event instead of a `MsgCreateClaim` event (see second screenshot below) 4. Fixed on-chain logging under `x/*` by replacing `logger\.(Info|Error)\("([^"]+)"(?:, (.*))?\)` with `logger.$1(fmt.Sprintf("$2", $3))` so it works as expected 5. Some quality-of-life comments & TODOs 6. Minor additions to unit tests from #220
Summary
Human Summary
This is aiming to fix multiple issues in E2E tests and just general QoL improvements:
relay.feature
about the need to runmake supplier1_stake && make app1_stake
until we fix [Configs] MakeGenesis
stakedActors
available via theAccountKeeper
#180 (known issue)sequencer-poktroll-sequencer
withlocalhost
(see first screenshot below)MsgSubmitProof
event instead of aMsgCreateClaim
event (see second screenshot below)x/*
by replacinglogger\.(Info|Error)\("([^"]+)"(?:, (.*))?\)
withlogger.$1(fmt.Sprintf("$2", $3))
so it works as expectedEventsReplayClient
Generic Event Listener forBlockClient
andDelegationClient
#220Reasoning for
s/sequencer-poktroll-sequencer/localhost
:Unresolved yet:
AI Summary
review pad:summary
Issue
Improving the
Type of change
Select one or more:
Testing
make go_develop_and_test
make e2e_test
Sanity Checklist