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

Fix goalNodeTest.exp #2781

Merged
merged 1 commit into from
Aug 23, 2021
Merged

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented Aug 20, 2021

Summary

Status code differs on CircleCI. Relaxed the status code value check.
Change the skipped expect tests to be defined as "t.Skip" so we can track these as such on the log file.

Test Plan

This is a test fix.

Comment on lines 117 to 118
"pingpongTest.exp": true, // broken
"listExpiredParticipationKeyTest.exp": true, // flaky
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to modify the expect test itself to return right away - so that the "disable" is on the test itself and we don't need to maintain "good test/bad test" lists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with this expected does not prints out output when it is successful so there is no way to know it did not run. With the current approach tho skipped files do not appear in go test -v log. Plus I'll add logging here to emphasize it was skipped.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the expect would output a "Skip" output ? we're already reading the output in the go-code, so we can just call the t.Skip in that case.

@algorandskiy algorandskiy force-pushed the pavel/expect-2 branch 2 times, most recently from 11b98b3 to e5cbda4 Compare August 20, 2021 20:44
* Disable listExpiredParticipationKeyTest as flaky one
* Increase timeout in goalClerkGroupTest
@tsachiherman
Copy link
Contributor

@algorandskiy - it's great that you're enabling these tests.. unfortunately these still randomly fails ( even with your fixes ).
I'm going to try and re-run the failed test and try to merge this one ( since it's better than before ), but I still think that we should disable the failed tests and create a ticket to handle these. I want to make sure we're allocating enough time to ourselves for addressing these issues, rather than addressing low hanging fruits.

@tsachiherman tsachiherman merged commit 3d41c6a into algorand:master Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants