Skip to content

Conversation

@algonautshant
Copy link
Contributor

@algonautshant algonautshant commented Oct 7, 2021

Summary

In testing.T, Error, Errorf, and Fail do not terminate the test.
Error and Errorf call Fail.
There will be another call to FailNow to terminate the test. (FailNow also calls Fail.)

When dontReportFailures is set in Error, Errorf, and Fail, the test will not be terminated.
Moreover, the subsequent call to FailNow will be ignored because of the set dontReportFailures flag.

In this change, dontReportFailures will not be set for Error, Errorf, and Fail so that the subsequent FailNow will terminate the test.

This issue is a fallout from #2844

Fixes #3020

Test Plan

This is a fix to the testing infrastructure.

In testing.T, Error, Errorf, and Fail do not terminate the test.
Error and Errorf call Fail.
There will be another call to FailNow to terminate the test. (FailNow also calls Fail.)

When dontReportFailures is set in Error, Errorf, and Fail, the test will not be terminated.
Moreover, the subsequent call to FailNow will be ignored because of the set dontReportFailures flag.

In this change, dontReportFailures will not be set for Error, Errorf, and Fail so that the subsequent FailNow will terminate the test.
@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2021

Codecov Report

Merging #3018 (bd2d1a3) into master (6887d0e) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3018      +/-   ##
==========================================
+ Coverage   43.84%   43.86%   +0.01%     
==========================================
  Files         385      385              
  Lines       86329    86329              
==========================================
+ Hits        37852    37865      +13     
+ Misses      42515    42504      -11     
+ Partials     5962     5960       -2     
Impacted Files Coverage Δ
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
catchup/service.go 69.35% <0.00%> (ø)
data/transactions/verify/txn.go 44.29% <0.00%> (ø)
network/requestTracker.go 70.68% <0.00%> (+0.43%) ⬆️
ledger/acctupdates.go 63.19% <0.00%> (+0.50%) ⬆️
data/abi/abi_type.go 91.81% <0.00%> (+0.90%) ⬆️
ledger/blockqueue.go 82.18% <0.00%> (+1.14%) ⬆️
network/wsPeer.go 73.57% <0.00%> (+2.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6887d0e...bd2d1a3. Read the comment docs.

@algonautshant algonautshant changed the title Don't set dontReportFailures for Fail Adjust the application of dontReportFailures in syncTest Oct 7, 2021
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

LGTM

@tsachiherman tsachiherman merged commit e00cde0 into algorand:master Oct 8, 2021
cce pushed a commit to cce/go-algorand that referenced this pull request Oct 28, 2021
In testing.T, Error, Errorf, and Fail do not terminate the test.
Error and Errorf call Fail.
There will be another call to FailNow to terminate the test. (FailNow also calls Fail.)

When `dontReportFailures` is set in Error, Errorf, and Fail, the test will not be terminated.
Moreover, the subsequent call to FailNow will be ignored because of the set dontReportFailures flag.

In this change, dontReportFailures will not be set for Error, Errorf, and Fail so that the subsequent FailNow will terminate the test.
@egieseke egieseke mentioned this pull request Nov 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.

Adjust the application of dontReportFailures in syncTest

4 participants