-
Notifications
You must be signed in to change notification settings - Fork 2
Promote find mmr entries and find trie entries to non IKWID commands #48
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
base: main
Are you sure you want to change the base?
Conversation
Also add prod based verification tests to ensure they work. RE: AB#10376
| "veracity", | ||
| "find-mmr-entries", | ||
| "--log-tenant", prodPublicTenant, | ||
| "--massif-start", "0", |
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 like this test, But setting the range to be zero length will likely excersise the short circute do nothing path. Just take this as a minor comment, I think these tests are great. Its hard to be sure we have enough massifs to do a non zero scan that ommits the massif containing the event
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 agree, we need to ramp up the public events :P I will revist the test when we have a later massifs in the prod public tenant.
It doesn't short circuit, it considers the first massif only (index 0) and there are two massifs
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.
although i agree that this test won't catch a short circuit if its introduced at a later date, so its not really a regression test for short circuiting at the 0 massif
| } | ||
|
|
||
| // TestEventsV1EventStdIn tests we can find | ||
| // the correct PROD eventsv1 event trie entry match. |
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.
Should we include the same tests we run against v2assetevents for eventsv1, namely:
// TestAssetsV2EventCorrectMassifStdIn tests we CAN find
// the correct PROD public assetsv2 event trie entry match
// if we set the range of massifs to include ONLY the massif the event is in.
// TestAssetsV2EventWrongMassifStdIn tests we CANNOT find
// the correct PROD public assetsv2 event trie entry match
// if we set the range of massifs to not include the massif the event is in.
?
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.
Will add them in :)
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.
good spot!
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.
aha yes i remember, we can't have these tests against prod, because we don't have access to a prod tenant with a large enough estate for their eventsv1 events to go over to a second massif.
It will be a gap in testing for sure until we get that, but the code uses the same code path for getting the massif as assetsv2 events, so i think its low risk, it just means future tinkerers will have to be careful about diverging that code path, and if they do hopefully we have a prod tenant estate large enough :)
Overview
Testing
RE: AB#10376