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: add bitcoind test without txindex enabled #4617

Merged
merged 4 commits into from
Oct 9, 2020
Merged

itest: add bitcoind test without txindex enabled #4617

merged 4 commits into from
Oct 9, 2020

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Sep 15, 2020

Fixes #1948.

Adds a secondary bitcoind integration test that runs without the -txindex flag set on the chain backend.

@guggero guggero added backend Related to the node backend software/interface (e.g. btcd, bitcoin-core) CI continuous integration v0.12 testing Improvements/modifications to the test suite travis Modifications to the Travis CI system labels Sep 15, 2020
@cfromknecht
Copy link
Contributor

Changes LGTM, was the build intentionally canceled?

@bhandras bhandras self-requested a review September 15, 2020 16:48
bhandras
bhandras approved these changes Sep 15, 2020
View reviewed changes
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

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

LGTM

@guggero
Copy link
Collaborator Author

guggero commented Sep 15, 2020

Haha, thanks for the reviews! I was still trying things out with this. I canceled the build because I made a mistake with the build flags, the btcd tests failed.

I'm not sure we can keep things as they are right now. Because Travis will only start 5 of the 6 tests in parallel. So this could end up taking more than an hour if the last test is only run after another is completed.

@bhandras
Copy link
Collaborator

Haha, thanks for the reviews! I was still trying things out with this. I canceled the build because I made a mistake with the build flags, the btcd tests failed.

I'm not sure we can keep things as they are right now. Because Travis will only start 5 of the 6 tests in parallel. So this could end up taking more than an hour if the last test is only run after another is completed.

Can we up our travis quota?

@ErikEk ErikEk mentioned this pull request Sep 21, 2020
Closed
@cfromknecht cfromknecht added this to In progress in v0.12.0-beta via automation Oct 5, 2020
@cfromknecht cfromknecht added this to the 0.12.0 milestone Oct 5, 2020
@cfromknecht
Copy link
Contributor

I'm not sure we can keep things as they are right now. Because Travis will only start 5 of the 6 tests in parallel. So this could end up taking more than an hour if the last test is only run after another is completed.

I think for now we should proceed with or without the bump in travis jobs. Maybe that will act as a forcing function...

Needs a rebase!!

@guggero
Copy link
Collaborator Author

guggero commented Oct 6, 2020

Rebased.

@guggero guggero moved this from In progress to Review in progress in v0.12.0-beta Oct 8, 2020
@halseth
Copy link
Contributor

halseth commented Oct 9, 2020

Doesn't compile?

@guggero
lntest: extract common bitcoind code into own file
43c2031 
To prepare for running multiple tests on bitcoind with different options
each time, we extract the common code into its own file.
@guggero
lntest: use extraArgs, fix linter issues
b7fc7c3
@guggero
lntest: add build flag for disabling txindex on bitcoind
3e3618a 
We create a new build flag for running the bitcoind tests without the
txindex enabled. We don't want this to be the default so we use a
negated build flag.
@guggero
travis: add bitcoind test without txindex
4dcdfd9
@guggero
Copy link
Collaborator Author

guggero commented Oct 9, 2020

Oops, fixed.

v0.12.0-beta automation moved this from Review in progress to Reviewer approved Oct 9, 2020
halseth
halseth approved these changes Oct 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 👍

@guggero guggero merged commit f98c743 into lightningnetwork:master Oct 9, 2020
v0.12.0-beta automation moved this from Reviewer approved to Done Oct 9, 2020
@guggero guggero deleted the no-txindex-itest branch October 9, 2020 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhandras bhandras bhandras approved these changes

@halseth halseth halseth approved these changes

@cfromknecht cfromknecht Awaiting requested review from cfromknecht

Assignees
No one assigned
Labels
backend Related to the node backend software/interface (e.g. btcd, bitcoin-core) CI continuous integration testing Improvements/modifications to the test suite travis Modifications to the Travis CI system v0.12
Projects
No open projects
v0.12.0-beta
  
Done
Milestone
0.12.0
Development

Successfully merging this pull request may close these issues.

No --txindex integration tests
4 participants
@guggero @cfromknecht @bhandras @halseth