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

multi: fix unit test flakes #7954

Merged

Conversation

yyforyongyu
Copy link
Member

Fix three uint test flakes,

--- FAIL: TestWebAPIFeeEstimator (5.00s)
    --- FAIL: TestWebAPIFeeEstimator/API-omitted_target (0.00s)
        estimator_test.go:253: 
                Error Trace:    /home/runner/work/lnd/lnd/lnwallet/chainfee/estimator_test.go:253
                Error:          Not equal: 
                                expected: 25300
                                actual  : 253
                Test:           TestWebAPIFeeEstimator/API-omitted_target
                Messages:       target 2 failed, fee map is map[20:101200 100:42]
    --- FAIL: TestWebAPIFeeEstimator/valid_target (0.00s)
        estimator_test.go:253: 
                Error Trace:    /home/runner/work/lnd/lnd/lnwallet/chainfee/estimator_test.go:253
                Error:          Not equal: 
                                expected: 25300
                                actual  : 253
                Test:           TestWebAPIFeeEstimator/valid_target
                Messages:       target 20 failed, fee map is map[20:101200 100:42]
    --- FAIL: TestWebAPIFeeEstimator/valid_target_extrapolated_fee (0.00s)
        estimator_test.go:253: 
                Error Trace:    /home/runner/work/lnd/lnd/lnwallet/chainfee/estimator_test.go:253
                Error:          Not equal: 
                                expected: 25300
                                actual  : 253
                Test:           TestWebAPIFeeEstimator/valid_target_extrapolated_fee
                Messages:       target 25 failed, fee map is map[20:101200 100:42]
FAIL
--- FAIL: TestHistoricalConfDetailsTxIndex (0.00s)
    --- FAIL: TestHistoricalConfDetailsTxIndex/rpc_polling_disabled (0.85s)
        bitcoind_test.go:174: should have found the transaction within the mempool, but did not: TxNotFoundIndex
FAIL
--- FAIL: TestTaprootBriefcase (0.03s)
    taproot_briefcase_test.go:87: 
            Error Trace:    /home/runner/work/lnd/lnd/contractcourt/taproot_briefcase_test.go:87
            Error:          Not equal: 
                            expected: &contractcourt.taprootBriefcase{CtrlBlocks:(*contractcourt.ctrlBlocks)(0xc000222820), TapTweaks:(*contractcourt.tapTweaks)(0xc0007b4b10)}
                            actual  : &contractcourt.taprootBriefcase{CtrlBlocks:(*contractcourt.ctrlBlocks)(0xc0002229b0), TapTweaks:(*contractcourt.tapTweaks)(0xc0007b4c60)}
                            
                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -11007,4 +11007,3 @@
                               },
                            -  BreachedHtlcTweaks: (contractcourt.htlcTapTweaks) {
                            -  },
                            +  BreachedHtlcTweaks: (contractcourt.htlcTapTweaks) <nil>,
                               BreachedSecondLevelHltcTweaks: (contractcourt.htlcTapTweaks) (len=154) {
            Test:           TestTaprootBriefcase

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Great fixes!
It looks like CI also ran into another one-in-however-million chances.

Here's the fix for the aezeed flake:

diff --git a/aezeed/cipherseed_test.go b/aezeed/cipherseed_test.go
index 7547f25e9..12c491966 100644
--- a/aezeed/cipherseed_test.go
+++ b/aezeed/cipherseed_test.go
@@ -533,7 +533,16 @@ func TestDecipherIncorrectMnemonic(t *testing.T) {
        // a checksum failure.
        swapIndex1 := 9
        swapIndex2 := 13
-       mnemonic[swapIndex1], mnemonic[swapIndex2] = mnemonic[swapIndex2], mnemonic[swapIndex1]
+       mnemonic[swapIndex1], mnemonic[swapIndex2] =
+               mnemonic[swapIndex2], mnemonic[swapIndex1]
+
+       // If the words happen to be the same by pure chance, we'll try again
+       // with different indexes.
+       if mnemonic[swapIndex1] == mnemonic[swapIndex2] {
+               swapIndex1 = 3
+               mnemonic[swapIndex1], mnemonic[swapIndex2] =
+                       mnemonic[swapIndex2], mnemonic[swapIndex1]
+       }
 
        // If we attempt to decrypt now, we should get a checksum failure.
        // If we attempt to map back to the original cipher seed now, then we


err string
name string
target uint32
apiEst uint32
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: apiEst is no longer used.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed!

chainntnfs/bitcoindnotify/bitcoind_test.go Show resolved Hide resolved
@Roasbeef Roasbeef added this to the v0.17.0 milestone Sep 4, 2023
Fix the following uint test flake,
```
--- FAIL: TestHistoricalConfDetailsTxIndex (0.00s)
    --- FAIL: TestHistoricalConfDetailsTxIndex/rpc_polling_enabled (1.16s)
        bitcoind_test.go:174: should have found the transaction within the mempool, but did not: TxNotFoundIndex
FAIL
```
When the numTweaks is zero, we should return a nil instead of
initializing an empty map as we'd get the following error,

```
Diff:
--- Expected
+++ Actual
@@ -11007,4 +11007,3 @@
},
-  BreachedHtlcTweaks: (contractcourt.htlcTapTweaks) {
-  },
+  BreachedHtlcTweaks: (contractcourt.htlcTapTweaks) <nil>,
```
@yyforyongyu yyforyongyu force-pushed the fix-chainfee-unit-test branch from 5d21d06 to 50f2c27 Compare September 4, 2023 23:44
@yyforyongyu yyforyongyu changed the base branch from 0-18-staging to master September 4, 2023 23:44
@yyforyongyu
Copy link
Member Author

@guggero thanks for the diff! Also interesting to see we hit the lottery here. Now let's see when we hit 1/2048^3🍀

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice, LGTM 🎉

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🕷️


// If the numTweaks happens to be zero, we return a nil to avoid
// initializing the map.
if numTweaks == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@Roasbeef Roasbeef merged commit fd58cbf into lightningnetwork:master Sep 5, 2023
@yyforyongyu yyforyongyu deleted the fix-chainfee-unit-test branch September 6, 2023 09:14
@carlaKC
Copy link
Collaborator

carlaKC commented Oct 10, 2023

Noticed a flake in TestWebAPIFeeEstimator on #8066 build, haven't looked into it deeply but def unrelated to the PR it's popped up in:

--- FAIL: TestWebAPIFeeEstimator (5.00s)
    --- FAIL: TestWebAPIFeeEstimator/target_w_too-low_fee (0.00s)
        estimator_test.go:252: 
            	Error Trace:	/home/runner/work/lnd/lnd/lnwallet/chainfee/estimator_test.go:252
            	Error:      	Not equal: 
            	            	expected: 500
            	            	actual  : 253
            	Test:       	TestWebAPIFeeEstimator/target_w_too-low_fee
            	Messages:   	target 106 failed, fee map is map[2:4000 6:2000]
    --- FAIL: TestWebAPIFeeEstimator/API-omitted_target (0.00s)
        estimator_test.go:252: 
            	Error Trace:	/home/runner/work/lnd/lnd/lnwallet/chainfee/estimator_test.go:252
            	Error:      	Not equal: 
            	            	expected: 1000
            	            	actual  : 253
            	Test:       	TestWebAPIFeeEstimator/API-omitted_target
            	Messages:   	target 1 failed, fee map is map[2:4000 6:2000]
    --- FAIL: TestWebAPIFeeEstimator/valid_target (0.00s)
        estimator_test.go:252: 
            	Error Trace:	/home/runner/work/lnd/lnd/lnwallet/chainfee/estimator_test.go:252
            	Error:      	Not equal: 
            	            	expected: 500
            	            	actual  : 253
            	Test:       	TestWebAPIFeeEstimator/valid_target
            	Messages:   	target 6 failed, fee map is map[2:4000 6:2000]

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.

4 participants