-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[nv16] propagate gas outputs from fvm + integrate safer ffi #8593
Conversation
3b6be70
to
a9b72d5
Compare
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.
Tests that are failing are already failing.
@@ -869,89 +868,6 @@ func TestAddPiece512MPadded(t *testing.T) { | |||
require.Equal(t, "baga6ea4seaqonenxyku4o7hr5xkzbqsceipf6xgli3on54beqbk6k246sbooobq", c.PieceCID.String()) | |||
} | |||
|
|||
func setupLogger(t *testing.T) *bytes.Buffer { |
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.
Could you explain why this needs to go entirely? I understand that generated
no longer exists, but has all this functionality vanished from the library? Or is it just behind different APIs? If the latter, we should adapt the tests, not drop the coverage.
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.
its gone completely afaict.
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.
It isn't. AFAIK no functionality has been removed, only APIs have changed. Here's where the log is now:
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.
ok feel free to reinstate, it is really ugly however.
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.
Agreed that the form of the test is terrible, but it is providing certain coverage that we can't drop here.
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.
Reinstated an adapted version here: 0d48654
(#8593)
5963613
to
24e68fe
Compare
1699d55
to
24e68fe
Compare
50825e4
to
b4dae33
Compare
See filecoin-project/ref-fvm#523
Depends on filecoin-project/filecoin-ffi#275
Integrates filecoin-project/filecoin-ffi#247.