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

P-chain UTs consolidation 2 - Context and Config #2121

Closed
wants to merge 52 commits into from

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented Oct 2, 2023

Why this should be merged

In #2107 we moved to a genesistest the machinery to build UTs genesis. Here we keep on going with vm.Config and vm.Ctx, moving them to a configtest package
Thanks @joshua-kim and @marun for the discussions that brought me to these changes.

How this works

Just refactor UTs setup

How this was tested

CI

@abi87 abi87 self-assigned this Oct 2, 2023
@abi87 abi87 force-pushed the pchain_UTs_setup_consolidation branch from 0a3cb46 to 079eac8 Compare October 2, 2023 09:25
@abi87 abi87 changed the title consolidated vm context in UTs setup P-chain UTs consolidataion - Context and Config Oct 2, 2023
@abi87 abi87 changed the title P-chain UTs consolidataion - Context and Config P-chain UTs consolidation - Context and Config Oct 2, 2023
@abi87 abi87 requested review from joshua-kim and marun October 2, 2023 09:35
@abi87 abi87 marked this pull request as ready for review October 2, 2023 09:35
@abi87 abi87 linked an issue Dec 9, 2023 that may be closed by this pull request

var (
fork = ts.DurangoFork
forkTime = ts.ValidateStartTime.Add(-2 * time.Second)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

different packages still have different fork times set. We'll fix this in next PR #2169

Comment on lines 85 to 102
var (
fork ts.ActiveFork
forkTime time.Time
)
switch {
case postDurango:
fork = ts.DurangoFork
forkTime = ts.ValidateStartTime.Add(-2 * time.Second).Add(-2 * time.Second)
case postCortina:
fork = ts.CortinaFork
forkTime = ts.ValidateStartTime.Add(-2 * time.Second).Add(-2 * time.Second)
case postBanff:
fork = ts.BanffFork
forkTime = ts.ValidateEndTime.Add(-2 * time.Second)
default:
fork = ts.ApricotPhase5Fork
forkTime = ts.GenesisTime
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

kind of arbitrary, but this is what we have been using fo far. Will be cleaned up in next PR #2169

Comment on lines 290 to 292
ApricotPhase3Time: ts.ValidateEndTime,
ApricotPhase5Time: ts.ValidateEndTime,
BanffTime: time.Time{}, // neglecting fork ordering this for package tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is partially fixed in this PR, meaning that at least we sort forks in the right order in testsetup package. In next PR #2169 we'll complete the fix by using the same fork times across different packages

Comment on lines 101 to 102
fork = ts.ApricotPhase5Fork
forkTime = ts.ValidateEndTime
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these values come from defaultConfig (now ditched in favour of ts.Config())

@abi87 abi87 requested a review from dhrubabasu as a code owner January 2, 2024 21:03
Comment on lines +55 to +64
func Context(tb testing.TB, baseDB database.Database) (*snow.Context, *MutableSharedMemory) {
ctx := snowtest.Context(tb, snowtest.PChainID)
m := atomic.NewMemory(baseDB)
msm := &MutableSharedMemory{
SharedMemory: m.NewSharedMemory(ctx.ChainID),
}
ctx.SharedMemory = msm

return ctx, msm
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moving this to its own package seems over fragmenting

},
ApricotPhase3Time: genesistest.ValidateEndTime,
ApricotPhase5Time: genesistest.ValidateEndTime,
BanffTime: time.Time{}, // neglecting fork ordering this for package tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I start changing this partially in this PR and finally fix fork times ordering in next PR

Comment on lines +82 to +99
var (
fork configtest.ActiveFork
forkTime time.Time
)
switch {
case postDurango:
fork = configtest.DurangoFork
forkTime = genesistest.ValidateStartTime.Add(-2 * time.Second).Add(-2 * time.Second)
case postCortina:
fork = configtest.CortinaFork
forkTime = genesistest.ValidateStartTime.Add(-2 * time.Second).Add(-2 * time.Second)
case postBanff:
fork = configtest.BanffFork
forkTime = genesistest.ValidateEndTime.Add(-2 * time.Second)
default:
fork = configtest.ApricotPhase5Fork
forkTime = genesistest.GenesisTime
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just a refactoring of the way we set fork times right now. We'll fix this in next PR

Copy link

github-actions bot commented Mar 3, 2024

This PR has become stale because it has been open for 30 days with no activity. Adding the lifecycle/frozen label will cause this PR to ignore lifecycle events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Status: In Review 👀
Development

Successfully merging this pull request may close these issues.

Make P-chain UTs maintanable
2 participants