lntest: allow the main test files to be buildable w/o the rpctest build tag #4593

guggero · 2020-09-04T11:51:14Z

This is the revival of #3382 originally created by @Roasbeef. I'm taking this over, I hope you don't mind.

In this PR, we modify our build tag set up to allow the main test
files to be buildable w/o the current rpctest tag. We do this so that
those of us that use extensions which will compile live files like
vim-go can once again fix compile errors as we go in our editors.

In order to do this, we now make an external testsCases variable, and
have two variants: one that's empty (no build tag), and one that's fully
populated with all our tests (build tag active). As a result, the main
file will now always build regardless of if the build tag is active or
not, but we'll only actually execute tests if the testCases variable
has been populated.

As sample run w/ the tag off:

=== RUN   TestLightningNetworkDaemon
--- SKIP: TestLightningNetworkDaemon (0.00s)
    lnd_test.go:13861: integration tests not selected with flag 'rpctest'
PASS
ok      github.com/lightningnetwork/lnd/lntest/itest    0.028s

halseth

Will be nice to finally get this in, LGTM 🎉

halseth · 2020-09-07T07:59:22Z

.travis.yml

-        - make lint workers=1
+        # Step 3: Lint go code. Limit to 1 worker and invoke GC more often to
+        # reduce memory usage.
+        - GOGC=20 make lint workers=1


Would it OOM on travis because of the increased number of lines to lint?

Yes. And if I look at GitHub Actions, it uses more than 8GB of RAM there when not restricted. As the lint on Travis only acts as a gatekeeper to prevent non-linted code to block our build queues, maybe it would make sense to exclude the itest directory there to speed things up? Or maybe there's a way to just include lines changed in the PR? I'll investigate.

Okay, restricting only to the commits of a PR doesn't speed things up. The linter will still analyze everything but only report problems added in the specified commits. I think excluding some directories might be the way to go. Or we just accept the 3 minutes this takes before each Travis run.

Yeah, I think that is okay. I would be interested to see if we could increase the number of workers on Travis though, as that would most likely be the way to speed it up (if it doesn't OOM)

Ah, good idea. If we can limit the amount of memory used through the environment variables, we should be able to increase the worker count again.
I looked it up on the Travis spec page and apparently the VMs have 7.5GB. I'm trying with GTOGC=50 and no worker limit which resulted in 6.3GB being used on my machine. Let's see if it OOMs with that on Travis or not.

Looking good:

$ GOGC=50 make lint ... INFO Memory: 396 samples, avg is 3384.2MB, max is 4855.0MB INFO Execution took 1m40.098082206s

cfromknecht · 2020-09-16T02:54:40Z

@guggero nice work in reviving this! needs a quick rebase then ready to go!

guggero · 2020-09-16T16:43:20Z

Rebased.

cfromknecht · 2020-09-16T22:40:05Z

Rebased.

Needs another :)

guggero · 2020-09-17T06:46:36Z

Rebased. If I had two green checkmarks, I could merge once all CI check pass ;-)

cfromknecht

LGTM 👏

This in prep for a bigger move in the next commit.

Otherwise, if we remove the build tags, then there's no default backend, and compilation will fail.

…ld tag In this commit, we modify our build tag set up to allow the main test files to be buildable w/o the current rpctest tag. We do this so that those of us that use extensions which will compile live files like vim-go can once again fix compile errors as we go in our editors. In order to do this, we now make an external `testsCases` variable, and have two variants: one that's empty (no build tag), and one that's fully populated with all our tests (build tag active). As a result, the main file will now always build regardless of if the build tag is active or not, but we'll only actually execute tests if the `testCases` variable has been populated. As sample run w/ the tag off: ``` === RUN TestLightningNetworkDaemon --- PASS: TestLightningNetworkDaemon (0.00s) PASS ok github.com/lightningnetwork/lnd/lntest/itest 0.051s ```

We fix all linter issues except for the 'lostcontext' and 'unparam' ones as those are too numerous and would increase the diff even more. Therefore we silence them in the itest directory for now. Because the linter is still not build tag aware, we also have to silence the unused and deadcode sub linters to not get false positives.

To make it possible to compile the itests together with the other tests, we don't want to use anything from the optional subservers.

There is a setting to control how often the garbage collector is run. Apparently this is a tradeoff between CPU and memory usage. If we can limit the memory being used in that way, this allows us to use multiple worker again, so overall this shouldn't be much slower than before.

guggero · 2020-09-21T19:19:17Z

Last linter run ended in OOM, reduced the GOGC variable from 50 to 30 to be on the safe side. If this run works, I'm going to merge.

Roasbeef · 2020-09-22T02:06:01Z

Super happy to see this finally land!

guggero added enhancement Improvements to existing features / behaviour golang/build system Related to the go language and compiler testing Improvements/modifications to the test suite labels Sep 4, 2020

guggero requested review from cfromknecht and halseth September 4, 2020 11:51

guggero added this to In progress in v0.12.0-beta via automation Sep 4, 2020

guggero mentioned this pull request Sep 4, 2020

lntest: allow the main test files to be buildable w/o the rpctest build tag #3382

Closed

guggero force-pushed the itest-linter-fix branch from 387cb11 to eaf63d0 Compare September 4, 2020 14:22

halseth approved these changes Sep 7, 2020

View reviewed changes

guggero force-pushed the itest-linter-fix branch from eaf63d0 to ba2adb9 Compare September 7, 2020 10:04

guggero force-pushed the itest-linter-fix branch from ba2adb9 to 874d611 Compare September 16, 2020 12:28

guggero force-pushed the itest-linter-fix branch from 874d611 to bf5dfe2 Compare September 17, 2020 06:46

guggero force-pushed the itest-linter-fix branch from bf5dfe2 to 91e9fb1 Compare September 17, 2020 09:52

cfromknecht approved these changes Sep 21, 2020

View reviewed changes

v0.12.0-beta automation moved this from In progress to Reviewer approved Sep 21, 2020

Roasbeef and others added 6 commits September 21, 2020 21:16

lntest/itest: move harness to new file

9563418

This in prep for a bigger move in the next commit.

lntest: default to btcd as default test harness backend

acd615a

Otherwise, if we remove the build tags, then there's no default backend, and compilation will fail.

lntest: make compilable without subserver build tags

9f7d8dd

To make it possible to compile the itests together with the other tests, we don't want to use anything from the optional subservers.

guggero force-pushed the itest-linter-fix branch from 91e9fb1 to 1558edb Compare September 21, 2020 19:17

guggero merged commit cd41cd4 into lightningnetwork:master Sep 21, 2020

v0.12.0-beta automation moved this from Reviewer approved to Done Sep 21, 2020

guggero deleted the itest-linter-fix branch September 21, 2020 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lntest: allow the main test files to be buildable w/o the rpctest build tag #4593

lntest: allow the main test files to be buildable w/o the rpctest build tag #4593

guggero commented Sep 4, 2020 •

edited

Loading

halseth left a comment

halseth Sep 7, 2020

guggero Sep 7, 2020

guggero Sep 7, 2020

halseth Sep 7, 2020

guggero Sep 7, 2020

guggero Sep 7, 2020

cfromknecht commented Sep 16, 2020

guggero commented Sep 16, 2020

cfromknecht commented Sep 16, 2020

guggero commented Sep 17, 2020

cfromknecht left a comment

guggero commented Sep 21, 2020

Roasbeef commented Sep 22, 2020

lntest: allow the main test files to be buildable w/o the rpctest build tag #4593

lntest: allow the main test files to be buildable w/o the rpctest build tag #4593

Conversation

guggero commented Sep 4, 2020 • edited Loading

halseth left a comment

Choose a reason for hiding this comment

halseth Sep 7, 2020

Choose a reason for hiding this comment

guggero Sep 7, 2020

Choose a reason for hiding this comment

guggero Sep 7, 2020

Choose a reason for hiding this comment

halseth Sep 7, 2020

Choose a reason for hiding this comment

guggero Sep 7, 2020

Choose a reason for hiding this comment

guggero Sep 7, 2020

Choose a reason for hiding this comment

cfromknecht commented Sep 16, 2020

guggero commented Sep 16, 2020

cfromknecht commented Sep 16, 2020

guggero commented Sep 17, 2020

cfromknecht left a comment

Choose a reason for hiding this comment

guggero commented Sep 21, 2020

Roasbeef commented Sep 22, 2020

guggero commented Sep 4, 2020 •

edited

Loading