-
Notifications
You must be signed in to change notification settings - Fork 135
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: add portal-spec-tests repo as submodule #1131
Conversation
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.
It would be useful to include the basic instructions for using submodules (eg. init / update) in the README
file as a reference. Basically what you posted in the trin discord channel
Created dedicated page in the book. Added short info and link to the README. |
@@ -121,9 +121,11 @@ mod test { | |||
|
|||
use super::*; | |||
|
|||
const TEST_DATA_DIRECTORY: &str = "../portal-spec-tests/tests/mainnet/state/serialization"; |
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'm sure this value will eventually be shared w/ other locations in the code. I'd recommend creating a ethportal-api/src/constants.rs
module to locate this value. Also, maybe change the var name to PORTAL_SPEC_TESTS_SUBMODULE_PATH
to be a bit more explicit in what it's pointing towards?
But... maybe it makes more sense to do something like ...
in ethportal-api/src/constants.rs
pub const PORTAL_SPEC_TESTS_SUBMODULE: &str = "../portal-spec-tests";
and then here, import PORTAL_SPEC_TESTS_SUBMODULE
and ...
const TEST_DATA_DIRECTORY = "/tests/mainnet/state/serialization";
and then join the values here? Since, it's likely other locations will want the path to the specs submodule, but don't necessarily want the same folder
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 created src/test_utils/*
and put it there, together with functions that reads file as string (less boilerplate everywhere).
I decided against ethportal-api/src/constants.rs
because that feels like a file that maybe we want to export publicly (as a crate), while this constant wouldn't make sense or fit there.
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.
The question I'd ask is, where are we likely to use this constant? According to #1130 we'll need it inside ethportal-peertest
, and I know we'll eventually need it inside portal-bridge
. So, maybe it makes sense that it's a constant we should export? But also, each workspace that needs assets from portal-spec-tests
can define it's own constant inside it's own workspace...
I'm undecided honestly, and it doesn't feel like a make or break decision here. So, let's just move ahead as is
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.
My worry is about its value to the crates that import it outside of this repo, like glados
and portal-hive
. It would just be meaningless in that scenario.
But considering that most frequent usage comes from this repo, I understand your point as well. And same as you, I'm undecided.
With that being said, I still prefer it in a test related path (not ethportal-api/src/constants.rs
). And if we decide to export it, we can just export it from there.
Rebased to head and cleaned up commit history. |
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 👍
What was wrong?
We couldn't use files from ethereum/portal-spec-tests directly in our tests. See: #1129
How was it fixed?
portal-spec-tests
as submoduleethportal-api/src/types/content_value/state.rs
to use those files instead.circleci/config.yml
to add support for submodules (instructions)fix: #1129
To-Do