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

test(x/ecocredit): move regen prefix configuration into module root #1253

Merged
merged 6 commits into from
Jul 14, 2022

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Jul 9, 2022

Description

Closes: #1243


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@ryanchristo
Copy link
Member

Thanks for the contribution @wgwz! This is a step in the right direction. The configuration was previously on a test setup basis so the next step will be updating the addresses used in tests that did not have the configuration.

You can run make test locally to see which tests are failing. You can also see which tests are currently failing by looking at the details of the CI job: https://github.com/regen-network/regen-ledger/runs/7265917088?check_suite_focus=true.

It looks like MsgCreate, MsgPut, and MsgTake in the basket submodule are the first set of tests failing, so you need to update the use of cosmos addresses to regen addresses in the feature files (e.g.basket/features/msg_create.feature).

cosmos1depk54cuajgkzea6zpgkq36tnjwdzv4afc3d27 -> regen1depk54cuajgkzea6zpgkq36tnjwdzv4ak663u6

After updating the addresses in the basket submodule, you'll find more tests failing in marketplace and elsewhere, so you'll need to update the addresses within the ecocredit module wherever tests are failing due to the use of a cosmos address.

@ryanchristo ryanchristo changed the title chore(x/ecocredit): move regen prefix configuration into root module test(x/ecocredit): move regen prefix configuration into root module Jul 10, 2022
@ryanchristo ryanchristo changed the title test(x/ecocredit): move regen prefix configuration into root module test(x/ecocredit): move regen prefix configuration into module root Jul 11, 2022
@wgwz
Copy link
Contributor Author

wgwz commented Jul 13, 2022

@ryanchristo thanks for the pointers! following those suggestions i think this should be a bit closer now. i think the tests were passing in my local environment. at the very least, i know i made some failures go away (i think... 😅 )

that said, i wanted to ask about some output i'm seeing when running the tests. i'm getting lots of warnings, should i be concerned about those? do we have known fixes/workarounds? i'm also seeing some stuff mark as "FAIL".

https://gist.github.com/wgwz/259eced61244492298bf5137e6a79ad1

@ryanchristo
Copy link
Member

that said, i wanted to ask about some output i'm seeing when running the tests. i'm getting lots of warnings, should i be concerned about those? do we have known fixes/workarounds? i'm also seeing some stuff mark as "FAIL".

Oh boy. I've never actually seen the output for tests when running on a Mac.

All the warnings related to go-keychain are ok to ignore. This has been a known issue and I think we're just waiting for the later version of the sdk although we might be able to use a replace directive in the meantime.

The failed app tests are also not an issue for me but they will be resolved with #1255. I didn't realize these were failing when running tests locally on a Mac but the reason makes sense.

The only errors I'm seeing now are these errors, which can be resolved by simply removing the github.com/cosmos/cosmos-sdk/types import in the files that no longer make use of the package.

@ryanchristo
Copy link
Member

Also, you'll need to resolve conflicts before the CI can run. The patch_test.go file was updated since you started working on this pull request. There's also a new init function in there that is no longer needed:

#1248 (comment)

@ryanchristo
Copy link
Member

Other than the import errors and resolving the conflicts, this looks great! Thank you!

@codecov
Copy link

codecov bot commented Jul 13, 2022

Codecov Report

Merging #1253 (43d7ae1) into master (8597f8c) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1253      +/-   ##
==========================================
+ Coverage   80.02%   80.11%   +0.09%     
==========================================
  Files         234      234              
  Lines       21556    21447     -109     
==========================================
- Hits        17250    17183      -67     
+ Misses       3023     2992      -31     
+ Partials     1283     1272      -11     
Flag Coverage Δ
experimental-codecov 80.11% <100.00%> (+<0.01%) ⬆️
stable-codecov ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
x/ecocredit/config.go 100.00% <100.00%> (ø)
types/coin.go 59.79% <0.00%> (-0.77%) ⬇️
app/stable_appconfig.go

Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Nice work. Looks good to me.

@wgwz
Copy link
Contributor Author

wgwz commented Jul 13, 2022

Thanks @ryanchristo feels good to have a contribution, thanks for the guidance on it :-)

@ryanchristo ryanchristo merged commit 75ec247 into regen-network:master Jul 14, 2022
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.

Use custom address prefix within feature files / unit tests
3 participants