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

[Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" #253

Merged
merged 21 commits into from
Dec 13, 2023

Conversation

Olshansk
Copy link
Member

@Olshansk Olshansk commented Dec 10, 2023

Summary

Human Summary

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 [Configs] Make Genesis staked Actors available via the AccountKeeper #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 [Off-Chain] Add EventsReplayClient Generic Event Listener for BlockClient and DelegationClient #220

Reasoning for s/sequencer-poktroll-sequencer/localhost:
Screenshot 2023-12-09 at 8 39 08 PM

Unresolved yet:
Screenshot 2023-12-09 at 8 51 20 PM

AI Summary

review pad:summary

Issue

Improving the

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: make e2e_test

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

@Olshansk Olshansk added the testing Test (or test utils) additions, fixes, improvements or other label Dec 10, 2023
@Olshansk Olshansk added this to the Shannon TestNet milestone Dec 10, 2023
@Olshansk Olshansk self-assigned this Dec 10, 2023
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Dec 10, 2023
@Olshansk Olshansk changed the title [Draft][Testing] E2E Test "Touchups" [Draft][Testing] Testing & Logging Fixes & "Touchups" Dec 10, 2023
Did a replace of this:
	logger\.(Info|Error)\("([^"]+)"(?:, (.*))?\)
With:
	logger.$1(fmt.Sprintf("$2", $3))
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

@Olshansk Olshansk marked this pull request as ready for review December 10, 2023 23:40
@Olshansk Olshansk requested review from okdas and removed request for red-0ne December 11, 2023 00:24
@Olshansk
Copy link
Member Author

@okdas Seems like the change I made from sequencer-poktroll-sequencer to localhost works when I run make test_e2e locally but fails in CI. Either I'm missing something or we might have to add an environment variable to configure this, please let me know what you suggest!

Screenshot 2023-12-10 at 4 25 46 PM

Screenshot 2023-12-10 at 4 25 23 PM

@Olshansk Olshansk requested a review from h5law December 11, 2023 18:33
Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

@Olshansk
Copy link
Member Author

@bryanchriswhite @h5law Please see my comment above regarding imports. I've re-validated that make go_test and make test_e2e work, and checking here now if it also works in devnet.

@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 👀 .

@Olshansk Olshansk changed the title [Draft][Testing] Testing & Logging Fixes & "Touchups" [Testing] Unit tests, E2E tests, logging and other Fixes / "Touchups" Dec 12, 2023
Copy link
Contributor

@h5law h5law left a 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

pkg/relayer/relayminer_test.go Outdated Show resolved Hide resolved
testutil/testclient/localnet.go Show resolved Hide resolved
x/supplier/client/cli/helpers_test.go Outdated Show resolved Hide resolved
@Olshansk
Copy link
Member Author

Olshansk commented Dec 12, 2023

@h5law See the comment I left here regarding imports. I'll make the manual changes suggested, but can you look into why tools/scripts/imports/main.go (executed via make go_imports) doesn't fix it.

…eviser -rm-unused -set-alias -format -output write
@bryanchriswhite
Copy link
Contributor

bryanchriswhite commented Dec 13, 2023

@Olshansk @h5law

  1. I resolved all import related comments without looking into it (see more details below).
  2. I tried to setup goimports-reviser but it did not work and opened an issue here.
  3. I ran make go_imports locally and am under the assumption it should solve what we need.
  4. If make go_imports does not fix issues introduced by my IDE, I suggest the following (in a followup PR):
  • Update go_imports
  • A PR that updates the imports
  • Document suggested import configs for poktroll developers

Please lmk if anything import-related is still blocking this PR.

BUMP:
image

Copy link
Contributor

@bryanchriswhite bryanchriswhite left a 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 🚢

localnet/kubernetes/values-relayminer.yaml Show resolved Hide resolved
Comment on lines +98 to +102
// 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")
Copy link
Contributor

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))
}

image

#Len()

func TestSanity(t *testing.T) {
	x := []struct{ a, b int }{{1, 2}, {3, 4}}
	require.Len(t, x, 3)
}

image

#Greater()

func TestSanity(t *testing.T) {
	x := []struct{ a, b int }{{1, 2}, {3, 4}}
	require.Greater(t, len(x), 2)
}

image

pkg/client/supplier/client.go Show resolved Hide resolved
@Olshansk Olshansk merged commit dd7df68 into main Dec 13, 2023
10 checks passed
@Olshansk Olshansk deleted the e2e_quick_fixes branch December 13, 2023 21:45
h5law added a commit that referenced this pull request Dec 14, 2023
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)
okdas pushed a commit that referenced this pull request Dec 19, 2023
…#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
@bryanchriswhite bryanchriswhite mentioned this pull request Dec 21, 2023
11 tasks
bryanchriswhite added a commit that referenced this pull request Dec 22, 2023
…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)
bryanchriswhite added a commit that referenced this pull request Dec 22, 2023
…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)
bryanchriswhite added a commit that referenced this pull request Dec 22, 2023
…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)
@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
…#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Test (or test utils) additions, fixes, improvements or other
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[Configs] Make Genesis staked Actors available via the AccountKeeper
4 participants