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

Tendermint JSON test case fixes #563

Merged
merged 9 commits into from
Sep 13, 2020
Merged

Tendermint JSON test case fixes #563

merged 9 commits into from
Sep 13, 2020

Conversation

greg-szabo
Copy link
Member

Closes #505

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

@greg-szabo greg-szabo mentioned this pull request Sep 11, 2020
5 tasks
@greg-szabo greg-szabo marked this pull request as ready for review September 13, 2020 16:53
@greg-szabo
Copy link
Member Author

All right, this is ready for review. Some of the light-client tests had to be disabled that were affected because of the change in voting power ordering or protobuf-encoding. The light-client integration test was also lowered to one iteration (essentially disabled) until the voting power change is implemented. (#556 )
All other tests were fixed but it would be better to regenerate the JSON files.

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.

IMO it could have been better to disable single test cases in some kind of blacklist. That way the JSON files could have served as a source for test-cases (s.t. we can guarantee the new generated tests to come will contain at least the cases that we already had).
On the other hand, deleting the failing cases is a bit cleaner.

FYI @Shivani912 and @andrey-kuprianov

.github/workflows/build.yml Show resolved Hide resolved
@@ -0,0 +1,287 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this supposed to be ignored given the line in the gitignore above? _*/*

Copy link
Member Author

Choose a reason for hiding this comment

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

No, the .gitignore only ignores the report folders that the test harness creates in case of failing tests. They look like this: _bisection/report, _simple/report, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, because the tests for the light client are now run with Testgen, it will ignore any files/folders starting with _

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it could have been better to disable single test cases in some kind of blacklist. That way the JSON files could have served as a source for test-cases (s.t. we can guarantee the new generated tests to come will contain at least the cases that we already had).
On the other hand, deleting the failing cases is a bit cleaner.

FYI @Shivani912 and @andrey-kuprianov

As I wrote above, Testgen will ignore any files or folders starting with _. So the easy way to disable a test or a set of tests, but to keep them at the same time, is to rename with _ in front.

@@ -0,0 +1,287 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Same question about gitgnore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same response. Gitignore only activates with an underscore in the folder name. Here, there is an underscore in the file name.

Copy link
Member

Choose a reason for hiding this comment

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

Ah OK. I see. Thanks for the clarification.

@greg-szabo
Copy link
Member Author

No test cases were deleted. The test harness reads a folder and ignores test JSON files that start with an underscore. I just renamed the test cases that failed with this underscore.

@greg-szabo greg-szabo merged commit a1d51fa into master Sep 13, 2020
@greg-szabo greg-szabo deleted the greg/test-fixes branch September 13, 2020 23:12
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.

Update JSON formatting to 0.34
3 participants