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

Add tests for Chain implementation #1093

Merged
merged 46 commits into from
Oct 2, 2020
Merged

Add tests for Chain implementation #1093

merged 46 commits into from
Oct 2, 2020

Conversation

yaahc
Copy link
Contributor

@yaahc yaahc commented Sep 24, 2020

Motivation

Solution

Related Issues

Related PRs

dconnolly
dconnolly previously approved these changes Sep 25, 2020
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

i love tests

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good!

Let's fix the slow tests issue before we merge, so we don't impact other developers.

Otherwise, this is all fantastic!

@teor2345
Copy link
Contributor

@yaahc what do we need to do to get this merged?

@teor2345
Copy link
Contributor

I merged #1090 into the state RFC, so we might have some conflicts here. Hopefully they will disappear in the rebase.

@yaahc
Copy link
Contributor Author

yaahc commented Sep 29, 2020

@yaahc what do we need to do to get this merged?

I think its just a bit of cleanup, make sure the tests dont run to wrong, etc. But first I want to try to see if I can't figure out why the sync is hanging when I use the full impl in the follow up PR

Co-authored-by: teor <teor@riseup.net>
@yaahc yaahc requested review from teor2345 and dconnolly September 30, 2020 21:10
@teor2345
Copy link
Contributor

@yaahc what do we need to do to get this merged?

I think its just a bit of cleanup, make sure the tests dont run to wrong, etc.

It would be nice to fix rustfmt before we merge.

But first I want to try to see if I can't figure out why the sync is hanging when I use the full impl in the follow up PR

Can you point me to the specific PR?
And any relevant failure logs?
(Unfortunately, sync hangs can be hard to diagnose - we need better futures tooling to find livelocked and deadlocked futures.)

There are a lot of PRs open right now, so I want to make sure I have the right one.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I think we're good here, as long as CI passes.

@yaahc yaahc merged commit 86ed130 into main Oct 2, 2020
@yaahc yaahc deleted the state-rfc-impl-tests branch October 2, 2020 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants