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

Adds more verification tests #475

Merged
merged 12 commits into from
Jul 29, 2020
Merged

Adds more verification tests #475

merged 12 commits into from
Jul 29, 2020

Conversation

Shivani912
Copy link
Contributor

Closes #330 - I think we should close this one now as all of the predicates are being tested by conformance tests. We can open a separate issue for the lack of unit tests for these.

  • 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

@romac
Copy link
Member

romac commented Jul 23, 2020

I guess this depends on #474?

@romac romac self-requested a review July 23, 2020 10:54
romac
romac previously approved these changes Jul 23, 2020
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot @Shivani912! Let's merge this after #474

@Shivani912
Copy link
Contributor Author

I guess this depends on #474?

Yes :)

Looks great, thanks a lot @Shivani912! Let's merge this after #474

Thanks for an early review @romac ! Will do...

@romac
Copy link
Member

romac commented Jul 27, 2020

@Shivani912 Any idea why the single_step_skipping test is failing now?

@Shivani912
Copy link
Contributor Author

@Shivani912 Any idea why the single_step_skipping test is failing now?

Aha, the older tests (tendermint/tests/lite.rs) is failing because we haven't fixed the predicate issue there. I had in mind that it's already deprecated. Are we still maintaining that code? @romac

@romac
Copy link
Member

romac commented Jul 29, 2020

Right, this is an old test, my bad! No we don't maintain that code anymore so I guess it's fine to just comment it out until we remove that code altogether.

@Shivani912 Shivani912 marked this pull request as ready for review July 29, 2020 13:30
@Shivani912
Copy link
Contributor Author

Thank you @romac for actively reviewing this PR, appreciate it! I have addressed all your comments and I think it's ready to merge now. Will be great if you could approve it to be able to merge. Thanks again !

@Shivani912 Shivani912 requested a review from romac July 29, 2020 13:49
@romac romac merged commit 667fc8d into master Jul 29, 2020
@romac romac deleted the shivani/verification_tests branch July 29, 2020 14:51
@romac
Copy link
Member

romac commented Jul 29, 2020

Great stuff, thanks @Shivani912!

@ebuchman
Copy link
Member

ebuchman commented Jul 30, 2020

Aren't all these tests being added in the old lite pkg? Do those files get run from the new light-client? It looks like they're duplicated in both the old and the new test dirs? A bit confused ...

Also, is there some way to see the mapping between the tests we have and the predicates? Like how should someone verify that there's a positive and negative test for each predicate?

It would be cool to run the Go code against these as well. Is there an easy way to do that?

And now that the mbt tools have landed, I guess next step is to start generating these tests from that?

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: Verification Testing
3 participants