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

chore!: refactor testnode to expose appcreator #1991

Merged
merged 3 commits into from
Jun 27, 2023

Conversation

evan-forbes
Copy link
Member

Overview

This refactor is mainly targeting #1990, but doing so in a way that uses common patters to avoid continuing to blow up the api

closes #1990

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@MSevey MSevey requested a review from a team June 27, 2023 04:34
Comment on lines +35 to +36
// AppCreator is used to create the application for the testnode.
AppCreator srvtypes.AppCreator
Copy link
Member Author

Choose a reason for hiding this comment

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

per #1990, we can now replace this function with a new one that creates a malicious applicaiton

@codecov-commenter
Copy link

codecov-commenter commented Jun 27, 2023

Codecov Report

Merging #1991 (9a4ce6c) into main (b2fc98c) will decrease coverage by 0.07%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1991      +/-   ##
==========================================
- Coverage   21.49%   21.43%   -0.07%     
==========================================
  Files         122      123       +1     
  Lines       13817    13856      +39     
==========================================
  Hits         2970     2970              
- Misses      10558    10597      +39     
  Partials      289      289              
Impacted Files Coverage Δ
test/util/testnode/config.go 0.00% <0.00%> (ø)
test/util/testnode/full_node.go 0.00% <0.00%> (ø)
test/util/testnode/node_interaction_api.go 0.00% <0.00%> (ø)
x/qgb/client/verify.go 0.00% <0.00%> (ø)

@evan-forbes evan-forbes self-assigned this Jun 27, 2023
@evan-forbes evan-forbes added the testing items that are strictly related to adding or extending test coverage label Jun 27, 2023
@evan-forbes evan-forbes added this to the Mainnet milestone Jun 27, 2023
Copy link
Contributor

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

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

Nice cleanup

// genesis state and consensus parameters. The provided keyring is used to
// create a validator key and the chainID is used to initialize the genesis
// state. The keyring is returned with the validator account added.
func InitFiles(
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a similar function in the e2e directory. Maybe as a followup we could look to consolidate

x/qgb/client/verify.go Show resolved Hide resolved
Copy link
Collaborator

@rootulp rootulp left a comment

Choose a reason for hiding this comment

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

Looks cleaner!

x/qgb/client/verify.go Show resolved Hide resolved
@evan-forbes evan-forbes merged commit ce0be95 into main Jun 27, 2023
@evan-forbes evan-forbes deleted the evan/refactor-testnode-appcreator branch June 27, 2023 20:26
evan-forbes added a commit that referenced this pull request Jul 3, 2023
## Overview

This refactor is mainly targeting #1990, but doing so in a way that uses
common patters to avoid continuing to blow up the api

closes #1990

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
evan-forbes added a commit that referenced this pull request Jul 3, 2023
## Overview

This refactor is mainly targeting #1990, but doing so in a way that uses
common patters to avoid continuing to blow up the api

closes #1990

## Checklist

- [x] New and updated code has appropriate documentation
- [x] New and updated code has new and/or updated testing
- [x] Required CI checks are passing
- [x] Visual proof for any user facing features like CLI or
documentation updates
- [x] Linked issues closed with keywords
evan-forbes added a commit that referenced this pull request Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing items that are strictly related to adding or extending test coverage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor testnode to expose appcreator
4 participants