Init test porting #394

RajarupanSampanthan · 2020-05-05T00:35:51Z

Summary of changes
Changes introduced in this pull request:

Ported Tests from https://github.com/filecoin-project/specs-actors/blob/master/actors/builtin/init/init_test.go
Some minor bugs were fixed along the way

Reference issue to close (if applicable)

Closes
310

Other information and links

…unction

austinabell

You're going to have to update from the address changes you pulled in (just removing the unwraps from id address creation) and make sure you run linter

austinabell · 2020-05-05T13:13:51Z

vm/actor/src/builtin/cron/mod.rs

+            let _v = rt.send(
                &entry.receiver,
                entry.method_num,
                &Serialized::default(),
                &TokenAmount::from(0u8),
-            )?;
+            );


revert back to what it was, this is ignoring that error

Yeah I was confused by this too. In the spec-actors repo they ignore any error or return values. I wasn't sure about why they did it but I just mimicked what they had. Are they doing something wrong?

ohh, you're right. Interesting why that error is ignored, but I guess it makes sense. Possibly add a comment here to indicate that this is intentional?

I would do let _ = instead of let _v = but yeah, so weird they dont use the error...

vm/actor/src/builtin/init/mod.rs

austinabell · 2020-05-05T13:21:05Z

vm/actor/tests/common/mod.rs

@@ -28,6 +28,7 @@ pub struct MockRuntime<'a, BS: BlockStore> {
    pub id_addresses: HashMap<Address, Address>,
    pub actor_code_cids: HashMap<Address, Cid>,
    pub new_actor_addr: Option<Address>,
+    pub message: UnsignedMessage,


So about this, since all you need from this is caller, receiver, value_received and is currently storing that data twice (see above) you either need to remove the three fields above and handle those through the message type or update the runtime interface to not be hard coded UnsignedMessage and instead be an interface which returns those fields.

I think it makes sense to just do the former and add a TODO since that change is a bit more involved (cc: @ec2)

Yeah it makes sense. I was thinking we need to make a Message trait. In this case, I think this is the only way to make it work. The message() function returns a reference to a UnsignedMessage, so its not like we can construct the message using the caller, receiever, value_received inside of that function

Just to be clear about that, it is a bit awkward to use, but only for the mock runtime. I'm not convinced that making it generic is objectively better, can just handle those things through an unsigned message in the mock rt since it's only used for testing, but maybe yeah a bit clunky still.

Having the message trait makes the mockrt impl more straightforward, at the cost of the actual runtime being unnecessarily complicated

Yeah. I was thinking about it from the perspective of the MockRt. I dont expect us to be building any more runtimes anyways.

vm/actor/tests/common/mod.rs

austinabell

Just a few small things

vm/actor/src/builtin/init/mod.rs

vm/actor/tests/common/mod.rs

vm/src/error.rs

…be used in message

vm/actor/tests/common/mod.rs

austinabell

last couple changes then good to go !

austinabell · 2020-05-07T15:29:10Z

vm/actor/src/builtin/init/mod.rs

@@ -90,6 +90,7 @@ impl Actor {
        rt.create_actor(&params.code_cid, &id_address)?;

        // Invoke constructor
+


nit

Suggested change

austinabell · 2020-05-07T15:29:33Z

vm/actor/tests/common/mod.rs

+            // UnsignedMessage::builder()
+            //     .to(receiver.clone())
+            //     .from(Address::default())
+            //     .value(0u8.into())
+            //     .build()
+            //     .unwrap(),


Suggested change

// UnsignedMessage::builder()

// .to(receiver.clone())

// .from(Address::default())

// .value(0u8.into())

// .build()

// .unwrap(),

austinabell · 2020-05-07T15:31:25Z

vm/actor/tests/init_actor_test.rs

+    rt.message = UnsignedMessage::builder()
+        .to(rt.message.to().clone())
+        .from(rt.message.from().clone())
+        .value(rt.message.value().clone())
+        .build()
+        .unwrap();


Suggested change

rt.message = UnsignedMessage::builder()

.to(rt.message.to().clone())

.from(rt.message.from().clone())

.value(rt.message.value().clone())

.build()

.unwrap();

austinabell · 2020-05-07T15:36:13Z

vm/actor/tests/init_actor_test.rs

+    let ret = match ret {
+        Ok(v) => Ok(v),
+        Err(e) => {
+            rt.state = prev_state;
+            Err(e)
+        }
+    };


Suggested change

let ret = match ret {

Ok(v) => Ok(v),

Err(e) => {

rt.state = prev_state;

Err(e)

}

};

if ret.is_err() {

rt.state = prev_state;

}

vm/actor/tests/init_actor_test.rs

RajarupanSampanthan and others added 14 commits April 21, 2020 20:25

Adding skeleton code and some functionality

83ebe74

Changing from private to public.

612d126

Added the construct and verification function. Tried to make a test f…

291d545

…unction

Some more changes.

2c67947

Some more changes.

3c09aa4

More test changes

450ff50

4 Tests are still failing

2cb23be

Fix in_transaction mock runtime field

a66432d

Clean up test file

9d2178e

Fix storage miner test

f421bd7

Fix payment channel tests

c3034d5

Fix abort test

5378d43

Init actor tests are passing

c878465

FIxed Cron actor test error

9beb783

RajarupanSampanthan requested review from ansermino, austinabell, dutterbutter and ec2 as code owners May 5, 2020 00:35

Merge branch 'master' into init_test_porting

9a8806d

dutterbutter added Status: Needs Review labels May 5, 2020

austinabell suggested changes May 5, 2020

View reviewed changes

RajarupanSampanthan added 2 commits May 6, 2020 00:50

Addding some changes suggets by AUstin

8e4f52d

FOrgot to add lint chnages

fd7280c

austinabell reviewed May 6, 2020

View reviewed changes

vm/actor/src/builtin/init/mod.rs Outdated Show resolved Hide resolved

vm/actor/src/builtin/init/mod.rs Outdated Show resolved Hide resolved

vm/actor/tests/common/mod.rs Outdated Show resolved Hide resolved

vm/actor/tests/common/mod.rs Outdated Show resolved Hide resolved

austinabell reviewed May 6, 2020

View reviewed changes

vm/src/error.rs Outdated Show resolved Hide resolved

Adding minor code changes and removed caller and reciever members to …

f6a74bd

…be used in message

austinabell suggested changes May 7, 2020

View reviewed changes

vm/actor/tests/common/mod.rs Outdated Show resolved Hide resolved

vm/actor/tests/common/mod.rs Outdated Show resolved Hide resolved

Adding some final touches

130153d

austinabell suggested changes May 7, 2020

View reviewed changes

ec2 reviewed May 7, 2020

View reviewed changes

vm/actor/tests/init_actor_test.rs Show resolved Hide resolved

ec2 reviewed May 7, 2020

View reviewed changes

vm/actor/tests/init_actor_test.rs Show resolved Hide resolved

Adding more cosmetic changes

dfcd35e

ec2 approved these changes May 7, 2020

View reviewed changes

Adding minor changes

12d8192

austinabell approved these changes May 7, 2020

View reviewed changes

austinabell added Status: Merge Ready and removed Status: Needs Review labels May 7, 2020

Merge branch 'master' into init_test_porting

b3c9d33

RajarupanSampanthan merged commit a048f25 into master May 8, 2020

RajarupanSampanthan deleted the init_test_porting branch May 8, 2020 04:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Init test porting #394

Init test porting #394

RajarupanSampanthan commented May 5, 2020

austinabell left a comment

austinabell May 5, 2020

RajarupanSampanthan May 5, 2020

austinabell May 6, 2020

ec2 May 6, 2020

austinabell May 5, 2020

ec2 May 5, 2020

austinabell May 5, 2020

ec2 May 5, 2020

austinabell left a comment

austinabell left a comment

austinabell May 7, 2020

austinabell May 7, 2020

austinabell May 7, 2020

austinabell May 7, 2020 •

edited

Loading

		@@ -90,6 +90,7 @@ impl Actor {
		rt.create_actor(&params.code_cid, &id_address)?;

		// Invoke constructor

Init test porting #394

Init test porting #394

Conversation

RajarupanSampanthan commented May 5, 2020

austinabell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell left a comment

Choose a reason for hiding this comment

austinabell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinabell May 7, 2020 • edited Loading

Choose a reason for hiding this comment

austinabell May 7, 2020 •

edited

Loading