Skip to content

Restrict the root-only tests to ./storage #2891

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

Merged
merged 1 commit into from
Jul 7, 2025

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Jul 7, 2025

The filter already assumes that subpackage, by grepping for a package-private function name, anyway.

Cc: @Luap99

@mtrmac mtrmac marked this pull request as ready for review July 7, 2025 16:43
@mtrmac mtrmac force-pushed the restrict-root-tests-storage branch from 5306dd0 to d415557 Compare July 7, 2025 16:55
@Luap99
Copy link
Member

Luap99 commented Jul 7, 2025

What is the motivation? I had it like this originally before I moved the logic into the Mekefile but then it seemed to complicated for me.

In particular how is this going to work like that, the makefile uses go test $(BUILDFLAGS) $(TESTFLAGS) -cover ./... so would it not always pass -cover ./... afterwards. Is that even a valid go command then?

@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 7, 2025

Great point about the ./..., thanks! (The way this seems to work is that it does restrict the tests to ./storage, but then it interprets the -cover ./... as options to be passed to the test, and I guess ignored.) I’ll rework this.

(Originally this started as a hack for other reasons), but ultimately it’s a bit unclean, and a bit of a waste, that we test+compile all of those sub packages where we know the filter will not match anything.

@Luap99
Copy link
Member

Luap99 commented Jul 7, 2025

(Originally this started as a hack for other reasons), but ultimately it’s a bit unclean, and a bit of a waste, that we test+compile all of those sub packages where we know the filter will not match anything.

Yeah fair enough, if the compile time is noticeable then this seems like an easy win indeed. And I guess the reduced log spam is also nice

@Luap99
Copy link
Member

Luap99 commented Jul 7, 2025

Great point about the ./..., thanks! (The way this seems to work is that it does restrict the tests to ./storage, but then it interprets the -cover ./... as options to be passed to the test, and I guess ignored.) I’ll rework this.

At the point of making it even more complicated I think the best thing would be to add yet another variable like TESTPACKAGE which defaults to ./... or something like that and then have the script set it?

The filter already assumes that subpackage, by grepping
for a package-private function name, anyway.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac mtrmac force-pushed the restrict-root-tests-storage branch from d415557 to f5c53d5 Compare July 7, 2025 17:12
@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 7, 2025

if the compile time is noticeable then this seems like an easy win indeed.

I’m not so worried about the compile / run time, with #2837 — if that ever lands. But “conceptually more precise”, “shorter logs” and “probably faster” seems worth doing, even if I started this for all the wrong reasons (to work around an unrelated bug in my work).

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 7, 2025

For the record, tests are now 6.5–7 minutes, down from 8.5–10 minutes in the previous commit.

@mtrmac mtrmac merged commit 778317a into containers:main Jul 7, 2025
10 checks passed
@mtrmac mtrmac deleted the restrict-root-tests-storage branch July 7, 2025 18:08
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.

2 participants