Skip to content
This repository has been archived by the owner on Jun 6, 2023. It is now read-only.

miner capacity upgrade scenario test #1030

Merged
merged 9 commits into from
Aug 25, 2020

Conversation

acruikshank
Copy link
Contributor

Motivation

We want to integration test more of the actor code, and a capacity upgrade for a sector containing deals is a good way to hit a lot of different actor functionality including the verified registry and market actor

Proposed Changes

  1. Add TestReplaceCommittedCapacitySectorWithDealLadenSector
  2. Move scenario tests into their own files named after flow under test instead of primary actor.
  3. Add public GetClaim method to power.State to permit retrieval power.
  4. Various small changes to test vm to ease testing.
  5. Additions to vm testing to advance till a sectors proving period and methods to inspect power state.

cc @wadealexc

Copy link
Member

@anorth anorth left a comment

Choose a reason for hiding this comment

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

This is good.

I think we should pause after this to write some helpers, especially around common-case params construction, to reduce noise. If we make a stateful component (i.e. that knows it's own address, next sector number etc), with no deps, it could be a component of the miner agent later.

actors/test/committed_capacity_scenario_test.go Outdated Show resolved Hide resolved
actors/test/committed_capacity_scenario_test.go Outdated Show resolved Hide resolved
actors/test/committed_capacity_scenario_test.go Outdated Show resolved Hide resolved
require.Equal(t, exitcode.Ok, code)

// assert successful precommit invocation
none := []vm.ExpectInvocation{}
Copy link
Member

Choose a reason for hiding this comment

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

Can you use nil? I'm guessing nil is interpreted as "don't care", but I think that would be fine here.

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 started out leaving subinvocations at the leaves untested, but I wasn't sure whether the leaves were actually leaves. This guarantees there are no unexpected calls between actors.


func init() {
var err error
VerifregRoot, err = address.NewIDAddress(80)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please follow up to put this in singletons.go too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

support/vm/testing.go Outdated Show resolved Hide resolved
@wadealexc
Copy link

Nice! Looks like you hit mostly the happy paths. It'd be good to also test the unhappy paths I mentioned - especially the one where deal init timeout results in a RestoreBytes invocation. That one's primarily Market actor functionality (CronTick), so it'd be good to cover it as well.

@acruikshank acruikshank force-pushed the feat/miner_capacity_upgrade_test branch from 1693ac3 to 3947c6b Compare August 25, 2020 18:21
@codecov-commenter
Copy link

codecov-commenter commented Aug 25, 2020

Codecov Report

Merging #1030 into master will decrease coverage by 0.0%.
The diff coverage is 60.0%.

@@           Coverage Diff            @@
##           master   #1030     +/-   ##
========================================
- Coverage    73.8%   73.8%   -0.1%     
========================================
  Files          57      57             
  Lines        6610    6615      +5     
========================================
+ Hits         4880    4883      +3     
- Misses       1108    1109      +1     
- Partials      622     623      +1     

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants