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

makefile: add unit test helpers #928

Merged
merged 7 commits into from
Jan 7, 2025

Conversation

ellemouton
Copy link
Member

Some helpers (lots of influence from the taproot-assets repo) to give devs more control over running unit tests.

With this, we can now do things like:

# Run all tests in the accounts package.
make unit pkg=accounts

# Run a specific test in accounts package
make unit pkg=accounts case=TestAccountService

# Pass in additional flags (more useful in later PRs). 
make unit tags=test_db_postgres

Add a new COMPILE_TAGS environment variable that includes all the basic
set of tags that lit requires in order for compilation to succeed. Let
LND_RELEASE_TAGS then extend these flags.

This will let us expand the testing_flags.mk file later without needing
to use the LND_RELEASE_TAGS variable there.
And use the new COMPILE_TAGS variable for the unit commands instead of
LND_RELEASE_TAGS.
@ellemouton ellemouton force-pushed the makeUnitTestHelpers branch from 574ddc8 to 0f6b014 Compare January 6, 2025 06:51
@ellemouton ellemouton force-pushed the makeUnitTestHelpers branch 2 times, most recently from a088437 to 4def4d2 Compare January 6, 2025 08:23
@ellemouton ellemouton self-assigned this Jan 6, 2025
Copy link
Contributor

@ViktorTigerstrom ViktorTigerstrom left a comment

Choose a reason for hiding this comment

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

Niiiice, thanks for this! tACK LGTM 🚀

make/testing_flags.mk Outdated Show resolved Hide resolved
make/testing_flags.mk Outdated Show resolved Hide resolved
This commit let's us specify commands of the form: `make unit
case=TestNameHere`. Note that this commit by itself is not very useful
in LiT since there are no tests in the main `terminal` package. A later
commit will let us set a package name too and then this will be more
useful.
This commit lets us perform commands of the form: `make unit
tags=postgres` to add additional tags to the command. This will be
useful in a future where we, for example, want to run unit tests against
a specific DB backend.
This commit lets us perform make commands of the following form:
`make unit pkg=accounts` which will run all the tests in the specified
package. Or you can do `make unit pkg=accounts case=TestAccountStore` to
run a specific test in the specified package.
Later on we will want to run the unit tests against different backends.
When that is the case, we can just add a new unit_type to this matrix
instead of needing to repeat all the same set-up steps.
@ellemouton ellemouton force-pushed the makeUnitTestHelpers branch from 4def4d2 to d6f184c Compare January 7, 2025 10:35
# If a specific package/test case was requested, run the unit test for the
# targeted case. Otherwise, default to running all tests.
ifeq ($(UNIT_TARGETED), yes)
UNIT := $(GOTEST) -tags="$(DEV_TAGS) $(COMPILE_TAGS)" $(TEST_FLAGS) $(UNITPKG)
Copy link
Contributor

Choose a reason for hiding this comment

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

make unit case=TestObfuscateConfig would not run, but only adding the package will add the test files in order for the test to be discoverable: make unit case=TestObfuscateConfig pkg=firewall. Do we want that, because it seems like an extra step that we could avoid?

Copy link
Member Author

Choose a reason for hiding this comment

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

note that this was mentioned in this commit message: 824a3c2

Copy link
Member Author

Choose a reason for hiding this comment

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

you have to specify package. Otherwise you have to search through all packages. Im sure there is a helper we can add later on but think this is fine for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

note that this was mentioned in this commit message: 824a3c2

ah yes, now that makes sense! I just noticed that in lnd it's the same (usually I'm running tests differently), so it makes sense to keep things consistent across projects. it really takes a lot longer if we'd include all packages with 24s vs 2s 👍

@ellemouton ellemouton requested a review from bitromortac January 7, 2025 11:55
@ellemouton ellemouton merged commit 90f9dfe into lightninglabs:master Jan 7, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants