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.

Sign up for GitHub

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

Jump to bottom

refactor: move duplicate utils to test utils crate #127

Merged
merged 1 commit into from
Jan 22, 2024
Merged

refactor: move duplicate utils to test utils crate #127

merged 1 commit into from
Jan 22, 2024

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented Jan 2, 2024 •
edited
Loading

@KolbyML KolbyML self-assigned this Jan 2, 2024
@KolbyML KolbyML marked this pull request as ready for review January 2, 2024 22:05
njgheorghita
njgheorghita reviewed Jan 8, 2024
View reviewed changes
Copy link
Contributor

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

Nice. These changes are looking good. One quick question, I left in a comment, about the location of the constants. And just to leave a note, not a blocker for this pr, but for future reference: It seems to me that this pr should've been broken up into multiple prs. Imo, the pr title should be relevant to all changes included in the pr. For sure, your commits were ordered and well-titled, which made things simple enough to review, but it's just easier to review when all code changes are logically relevant to the pr they're included in. I get that it's less efficient than lumping together 4 minor refactors into a single pr, but it's better practice imo, and makes reviewing easier.

simulators/history/rpc-compat/src/main.rs Outdated
@@ -469,10 +600,16 @@ dyn_async! {

dyn_async! {
// test that a node will return a AbsentContent via RecursiveFindContent when the data doesn't exist
async fn test_recursive_find_content_content_absent<'a> (client: Client) {
async fn test_recursive_find_content_content_absent<'a>(clients: Vec<Client>, _: Option<Vec<(String, String)>>) {
let client = match clients.into_iter().next() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe writing a small utility function could be useful here to cut down on some of the repeated code?

eg...

fn get_expected_client(clients: Vec<Client>) -> Client {
    match clients.into_iter().next() {
       Some((client)) => client,
       None => {
           panic!("Unable.... ");
       }
    }
}

Or maybe there's a cleaner solution I'm missing, just seems to me like there's an opportunity here to get rid of some of the repeated code.

simulators/history/portal-interop/Cargo.toml
@@ -6,6 +6,7 @@ edition = "2021"

[dependencies]
ethportal-api = { git = "https://github.com/ethereum/trin", rev = "2a32224e3c2b0b80bc37c1b692c33016371f197a" }
portal-spec-test-utils-rs = { git = "https://github.com/ethereum/portal-spec-tests", rev = "d1e996d0d4dc2136b3cd38d9e25cdc3a6b74dcd9" }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, but is storing these constants in a separate repository necessary? Seems to me like we'd like to store them in this repo, to make it easier to add/remove/edit constants, rather than juggling them between multiple repositories. Is there any reason why we couldn't just create a "utils" workspace inside portal-hive?

Copy link
Contributor

Choose a reason for hiding this comment

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

@KolbyML I'm still curious about this question

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can store it in the simulator folder I will check

Copy link
Contributor

@njgheorghita njgheorghita Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Honestly, I'm using these constants right now in some scripting work I'm doing in trin, so maybe we just store them in trin-utils trin_validation::constants?

simulators/README.md Outdated
- ``beacon-state``
- ``history-state``

## Examples of invlaid folder names :x:
Copy link
Contributor

Choose a reason for hiding this comment

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

invalid

simulators/README.md Outdated
@@ -0,0 +1,21 @@
# Portal Simulators
Portal simulators are sorted by folders indictacting what networks are being tested.
Copy link
Contributor

Choose a reason for hiding this comment

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

indicating

simulators/README.md Outdated
# Portal Simulators
Portal simulators are sorted by folders indictacting what networks are being tested.

The names of folder are
Copy link
Contributor

Choose a reason for hiding this comment

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

The names of the folders are...

simulators/README.md Outdated

The names of folder are
- all lowercase
- dash seperated
Copy link
Contributor

Choose a reason for hiding this comment

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

separated

@KolbyML
Copy link
Member Author

KolbyML commented Jan 8, 2024 •
edited
Loading

I didn't mark which PR's should be merged when/reviewed so here is the order.

  1. fix: make gossip portal-interop test auto generate comments for errors #118
  2. refactor: move history hive tests to history folder add instructions for where to put simulators #124
  3. refactor: clean up client bash files and add configurable networks #125
  4. feat: update rpc_compat to use NClientTestSpec #126
  5. refactor: move duplicate utils to test utils crate #127

I should better document this next time, I really appreiate the review.

This was referenced Jan 8, 2024
Merged
Merged
Merged
@KolbyML
Copy link
Member Author

KolbyML commented Jan 18, 2024

@njgheorghita this PR is ready for another review

njgheorghita
njgheorghita approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@njgheorghita njgheorghita left a comment

Choose a reason for hiding this comment

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

🚢

simulators/history/portal-interop/Cargo.toml
@@ -6,6 +6,7 @@ edition = "2021"

[dependencies]
ethportal-api = { git = "https://github.com/ethereum/trin", rev = "2a32224e3c2b0b80bc37c1b692c33016371f197a" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is portal-hive tagged to a particular hash of ethportal-api? If possible I think it's much more preferable to tag to the version

Copy link
Member Author

Choose a reason for hiding this comment

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

Because ethportal-api has broken in the past and has broken our builds.

Sure that would be a fine change to make in an upcoming PR

Copy link
Member Author

Choose a reason for hiding this comment

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

I will make an issue for it

@KolbyML KolbyML mentioned this pull request Jan 22, 2024
Open
@KolbyML KolbyML merged commit f61b79f into ethereum:main Jan 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njgheorghita njgheorghita njgheorghita approved these changes

@carver carver Awaiting requested review from carver

@ogenev ogenev Awaiting requested review from ogenev

Assignees

@KolbyML KolbyML

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@KolbyML @njgheorghita