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

Conformance test fixes #366

Merged
merged 4 commits into from
Jun 23, 2020
Merged

Conformance test fixes #366

merged 4 commits into from
Jun 23, 2020

Conversation

Shivani912
Copy link
Contributor

@Shivani912 Shivani912 commented Jun 23, 2020

closes #290
partially addresses: #320 (bisection)

UPDATE: moved single_peer/invalid_validator_set to multi_peer/malicious_validator_set because it seemed more appropriate.

QUESTION: Why do we don't run the multi_peer/... tests yet?

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGES.md

@Shivani912 Shivani912 requested review from romac and brapse June 23, 2020 19:16
@liamsi
Copy link
Member

liamsi commented Jun 23, 2020

QUESTION: Why do we don't run the multi_peer/... tests yet?

I think when you had them ready and generated the JSON files, we added them to this repo without the rust code being ready?

async fn bisection() {
// TODO: re-enable multi-peer tests as soon as the light client can handle these:
// let dir = "bisection/multi_peer";
// run_bisection_tests(dir).await;

@Shivani912
Copy link
Contributor Author

QUESTION: Why do we don't run the multi_peer/... tests yet?

I think when you had them ready, we added them to this repo without the rust code being ready?

async fn bisection() {
// TODO: re-enable multi-peer tests as soon as the light client can handle these:
// let dir = "bisection/multi_peer";
// run_bisection_tests(dir).await;

Possible, multi_peer tests fail at the moment in rust code. Seems to me it's not fetching headers from a witness. Will have to take a closer look though!

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I think it would be good if the missing multipeer tests were captured in an issue (if not already the case)

@Shivani912
Copy link
Contributor Author

Thanks @liamsi ! Will do that :)

@Shivani912 Shivani912 merged commit e6ac367 into master Jun 23, 2020
@Shivani912 Shivani912 deleted the shivani/test-fixes branch June 23, 2020 20:28
@brapse brapse mentioned this pull request Jul 14, 2020
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.

Light client conformance tests differences between description and real errors
2 participants