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.

Sign up for GitHub

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

Jump to bottom

itest: clean harness state before each icase and better naming for icase logs #4737

Merged
merged 4 commits into from
Nov 12, 2020
Merged

itest: clean harness state before each icase and better naming for icase logs #4737

merged 4 commits into from
Nov 12, 2020

Conversation

bhandras
Copy link
Collaborator

This PR changes the itest harness to make it possible to restart the harness nodes (Alice & Bob) before each icase while also changing the way we name icase node logs, to make it easier to see which logs belong to which icase.

@bhandras bhandras requested a review from guggero October 29, 2020 15:15
@bhandras bhandras requested a review from cfromknecht October 29, 2020 15:32
guggero
guggero reviewed Oct 30, 2020
View reviewed changes
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.

Solid start, this will hopefully crush some more flakes!

Great idea with the log file name, this should make it much easier to find what we're looking for.

lntest/harness.go Outdated Show resolved Hide resolved
lntest/harness.go Outdated Show resolved Hide resolved
lntest/itest/lnd_test.go Outdated Show resolved Hide resolved
lntest/itest/lnd_test.go Show resolved Hide resolved
lntest/harness.go Show resolved Hide resolved
@bhandras bhandras requested a review from guggero October 30, 2020 09:49
guggero
guggero reviewed Oct 30, 2020
View reviewed changes
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.

Almost there, just two more small changes and this is good to go IMO.

lntest/node.go Outdated Show resolved Hide resolved
lntest/itest/lnd_test.go Outdated Show resolved Hide resolved
@bhandras bhandras requested a review from guggero October 30, 2020 12:00
guggero
guggero approved these changes Oct 30, 2020
View reviewed changes
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.

LGTM 💯

@bhandras bhandras requested review from halseth and removed request for cfromknecht October 30, 2020 12:04
@bhandras bhandras mentioned this pull request Oct 30, 2020
Merged
@bhandras bhandras requested a review from guggero October 30, 2020 16:55
@bhandras
Copy link
Collaborator Author

bhandras commented Oct 30, 2020 •
edited
Loading

Had to add one more commit because it seems that the parallel macaroon itests crash with the restarted nodes. @guggero PTAL
edit: will squash with prev commit if approved

@guggero
Copy link
Collaborator

guggero commented Nov 2, 2020

Ah, interesting. Because the outer t.Run() now includes the setup/teardown, we cannot use t.Parallel() in a sub test directly (unless we wrap it in another sequential t.Run() block).

https://stackoverflow.com/questions/53950205/how-to-handle-parent-test-teardown-with-parallel-subtests-in-golang

@bhandras
Copy link
Collaborator Author

bhandras commented Nov 2, 2020

Ah, interesting. Because the outer t.Run() now includes the setup/teardown, we cannot use t.Parallel() in a sub test directly (unless we wrap it in another sequential t.Run() block).

https://stackoverflow.com/questions/53950205/how-to-handle-parent-test-teardown-with-parallel-subtests-in-golang

yeah, fortunately there wasn't much gain from parallelizing those tests anyway

@bhandras
Copy link
Collaborator Author

bhandras commented Nov 2, 2020 •
edited
Loading

The btcd and neutrino itests still seem to flake a lot on travis... Looking into why this happens.

Edit: added another commit, it seems to be a timeout. Testing that hypothesis...

@bhandras bhandras force-pushed the itest_clean_state branch 2 times, most recently from ccc827c to b02d09f Compare November 2, 2020 13:57
halseth
halseth reviewed Nov 2, 2020
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

I think the concept is good :) I'm just wondering if there are benefits to having long-running nodes being part of the test cases (makes us notice state that is not propely cleaned up perhaps), but all in all I think this is better :)

lntest/itest/lnd_macaroons_test.go Show resolved Hide resolved
make/testing_flags.mk Outdated Show resolved Hide resolved
@bhandras bhandras requested a review from halseth November 2, 2020 14:50
@bhandras
Copy link
Collaborator Author

bhandras commented Nov 2, 2020

I think the concept is good :) I'm just wondering if there are benefits to having long-running nodes being part of the test cases (makes us notice state that is not propely cleaned up perhaps), but all in all I think this is better :)

Yeah, good question which is better. I lean towards clean state (hence the PR). Perhaps we could have a longer test that concatenates itests randomly or even in a predefined way. But maybe not the whole test-suite. I suppose the initial goal was to spare some time restarting the nodes, but since #4655 is just around the corner we can be a bit more strict with state.

@bhandras bhandras added this to the 0.12.0 milestone Nov 3, 2020
@bhandras bhandras added the itests Issues related to integration tests. label Nov 3, 2020
halseth
halseth reviewed Nov 4, 2020
View reviewed changes
lntest/itest/lnd_test.go Show resolved Hide resolved
lntest/node.go Show resolved Hide resolved
lntest/itest/lnd_macaroons_test.go Show resolved Hide resolved
@halseth
Copy link
Contributor

halseth commented Nov 5, 2020

Needs rebase now that #4655 is in!

@bhandras
Copy link
Collaborator Author

bhandras commented Nov 6, 2020

Needs rebase now that #4655 is in!

Rebased, I hope this eliminates most of the flakes.

@bhandras bhandras requested a review from halseth November 6, 2020 13:24
guggero
guggero approved these changes Nov 6, 2020
View reviewed changes
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.

One last small thing then I think we're good to go!

lntest/itest/lnd_test.go Outdated Show resolved Hide resolved
halseth
halseth approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM 👍

make/testing_flags.mk Outdated Show resolved Hide resolved
@bhandras
Copy link
Collaborator Author

bhandras commented Nov 9, 2020

I'm running some experiments with new chainbackend and miner for each testcase, as right now we're still pretty unstable.

@guggero
Copy link
Collaborator

guggero commented Nov 10, 2020

I think btcsuite/btcwallet#695 should also help with the neutrino rescan stability. Unfortunately there seems to be a deadlock in there but I can't figure out where exactly (itests don't even start properly if I use that branch of btcwallet).

@bhandras bhandras force-pushed the itest_clean_state branch 2 times, most recently from aac6172 to e832efe Compare November 11, 2020 14:20
@bhandras
itest: use require.NoError in TestLightningNetworkDaemon
d6c2ee1
@bhandras
itest: add icase to node log filename + restart nodes before each icase
979c849 
This commit adds the icase name to the log filename, to make it simpler
to find problematic tests. Additionally after this commit we'll restart
Alice and Bob (the base harness nodes) before each icase to start with a
clean state.
@bhandras
itest: fix crash in parallel macaroon tests
9eac0dd
@bhandras
itest: update error log whitelist
38e8184
@guggero guggero mentioned this pull request Nov 12, 2020
Merged
@halseth
Copy link
Contributor

halseth commented Nov 12, 2020

Gonna merge, as the flakes seem unrelated to these changes, and it unblocks some other flake-fixing PRs.

@halseth halseth merged commit 3f9a707 into lightningnetwork:master Nov 12, 2020
@bhandras bhandras deleted the itest_clean_state branch September 12, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guggero guggero guggero approved these changes

@halseth halseth halseth approved these changes

Assignees
No one assigned
Labels
itests Issues related to integration tests.
Projects
None yet
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bhandras @guggero @halseth