Skip to content

Conversation

yhabteab
Copy link
Member

@yhabteab yhabteab commented Sep 4, 2025

It's kinda annoying that the tests spams the GHA logs with lots of log messages even though the tests aren't failing, see 1 as an example. Since zap already provides a logger that logs to the testing.T instance, allow overriding the core factory to use that logger instead. This way, the logs are only visible when the test fails or when running the tests with the -v flag.

Tests

~/Workspace/go/icinga-go-library (logging-with-factory ✗) go test './logging/...'
ok  	github.com/icinga/icinga-go-library/logging	0.212s
~/Workspace/go/icinga-go-library (logging-with-factory ✗) go test -v './logging/...'
...
=== RUN   TestNewLoggingWithFactory
=== PAUSE TestNewLoggingWithFactory
=== CONT  TestNewLoggingWithFactory
    logger.go:146: 2025-09-04T17:38:01.177+0200	DEBUG	my-component	This is a debug message
    logger.go:146: 2025-09-04T17:38:01.177+0200	INFO	my-component	This is an info message
    logger.go:146: 2025-09-04T17:38:01.177+0200	DEBUG	my-component.child	This is a debug message from child
--- PASS: TestNewLoggingWithFactory (0.00s)
PASS
ok  	github.com/icinga/icinga-go-library/logging	0.249s

Footnotes

  1. https://github.com/Icinga/icinga-notifications/actions/runs/17466069738/job/49602019404?pr=270

It's kinda annoying that the tests spams the GHA logs with lots of log
messages even though the tests aren't failing, see [^1] as an example.
Since zap already provides a logger that logs to the testing.T instance,
allow overriding the core factory to use that logger instead. This way,
the logs are only visible when the test fails or when running the tests
with the `-v` flag.

[^1]: https://github.com/Icinga/icinga-notifications/actions/runs/17466069738/job/49602019404?pr=270
@yhabteab yhabteab requested a review from oxzi September 4, 2025 15:40
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Sep 4, 2025
@yhabteab
Copy link
Member Author

yhabteab commented Sep 4, 2025

Oh, the tests on GitHub are of course run with the -v flag, this will only be relevant when running the tests locally.

Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

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

Oh, the tests on GitHub are of course run with the -v flag, this will only be relevant when running the tests locally.

I am unsure, but wouldn't just dropping -v do the trick?

Comment on lines 3 to 10
import (
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
"os"
"sync"
"time"

"go.uber.org/zap"
"go.uber.org/zap/zapcore"
)
Copy link
Member

Choose a reason for hiding this comment

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

There is no reason to reorder the imports.

Most (or all?) Icinga Go code just uses one big import block. While I personally dislike this pattern and would prefer grouped imports, it should at least be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not going over all the files now and group some random imports, I didn't even do this change by hand. Each time I edit a go file Goland do this automatically and I've to literally play cat and mouse with it, so that I can commit other relevant changes without this, but it's too annoying, so I gave up. And no, I'm not using any special IDE flags to enforce that grouping, like I said last time, it just seem to happen since the last upgrade.

Bildschirmfoto 2025-09-05 um 09 44 53

As you can see, it just uses the simple goimports command without the Group packages from Go SDK option set, so 🤷‍♂️!

Copy link
Member

Choose a reason for hiding this comment

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

Don't use goimports but gofmt. Maybe this is new?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm maybe, looking at the goimports command source code, its format-only option says this:

if true, don't fix imports and only format. In this mode, goimports is effectively gofmt, with the addition that imports are grouped into sections.

@yhabteab
Copy link
Member Author

yhabteab commented Sep 5, 2025

Oh, the tests on GitHub are of course run with the -v flag, this will only be relevant when running the tests locally.

I am unsure, but wouldn't just dropping -v do the trick?

The trick? Dropping the -v flag will only hide outputs written into the testing.T instance, and we use just regular console loggers, so this PR is supposed to allow just this. With this PR, the unittests can use a zaptest logger which forwards everything to the provided testing.T instance, and so the output can be controlled with the -v flag just like regular test outputs.

@yhabteab
Copy link
Member Author

yhabteab commented Sep 5, 2025

Oh, the tests on GitHub are of course run with the -v flag, this will only be relevant when running the tests locally.

I am unsure, but wouldn't just dropping -v do the trick?

The trick? Dropping the -v flag will only hide outputs written into the testing.T instance, and we use just regular console loggers, so this PR is supposed to allow just this. With this PR, the unittests can use a zaptest logger which forwards everything to the provided testing.T instance, and so the output can be controlled with the -v flag just like regular test outputs.

Ok, I'm noticing now that the existing incident tests do indeed use a zaptest logger, the problem was just due to a not yet pushed code of mine, which just uses a regular console logger. Nonetheless, it would be nice to have this helper function as it allows to construct a test Logging instance, which is required by the config.NewRuntimeConfig function in Icinga Notifications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants