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

miner actor: construction tests #42

Merged
merged 11 commits into from
Mar 7, 2022
Merged

miner actor: construction tests #42

merged 11 commits into from
Mar 7, 2022

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Mar 4, 2022

Unit tests for #27

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM, once it passes

Comment on lines +132 to +134
for i in 0..WPOST_PERIOD_DEADLINES {
let c = deadlines.due[i as usize];
Copy link
Member

Choose a reason for hiding this comment

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

You can just iterate over deadlines.due. Or deadlines.due.iter().enumerate()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I was following the go test and didn't think rust enough.

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 think i'll keep it braindead to be as close to the original test as possible.

@vyzo
Copy link
Contributor Author

vyzo commented Mar 4, 2022

rebased on master for the workflow thing.

@Stebalien Stebalien marked this pull request as ready for review March 5, 2022 13:47
@vyzo
Copy link
Contributor Author

vyzo commented Mar 7, 2022

Added the remaining miner construction tests and did a bit of squashing.

@vyzo vyzo changed the title WIP: miner actor tests miner actor: construction tests Mar 7, 2022
@vyzo vyzo requested a review from Stebalien March 7, 2022 09:44
@vyzo vyzo mentioned this pull request Mar 7, 2022
&RawBytes::serialize(params).unwrap(),
)
.unwrap_err();
assert_eq!(result.exit_code(), ExitCode::ErrIllegalArgument);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR but one thing we are missing compared to go tests is checking substrings of the error. I'd like to see us add an analogue of ExpectAbortContainsMessage. What do you think @vyzo? I know you aren't a big fan of checking error strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dunno, i really dont like parsing error messages, but maybe it makes sense here.

Lets open issue to keep track of this?

actors/miner/tests/miner_actor_test_construction.rs Outdated Show resolved Hide resolved
@vyzo vyzo merged commit b734918 into master Mar 7, 2022
@vyzo vyzo deleted the test/miner-actor branch March 7, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants