Update to k6-core master and fix logCache logger #842
Conversation
tests/logrus_hook.go
Outdated
| if ll, ok := fl.(*logrus.Logger); ok { | ||
| logger = ll | ||
| } else { | ||
| logger = logrus.New() |
There was a problem hiding this comment.
Would it be a good idea to log a warning as we do in #824 when fl is not the expected type?
There was a problem hiding this comment.
I thought that this was unnecessary considering that this is only used in integration tests, and we'll probably ignore it anyway and just focus on the test results. But if you still think it can be useful for some cases let me know and we can add it 👍
There was a problem hiding this comment.
Tbh, I don't fully understand how it's used, so it's why i'm leaning towards making things clearer with a log message. But, if you know what's going on then i'm happy to leave it as is.
There was a problem hiding this comment.
Actually, reviewing this to better explain the context, I think the implementation is not exactly correct, as using the default logger here (in case the provided by state.Logger is not of type *logrus.Logger), will make the tests fail because that "default logger" is in the end not used by our implementation, so no hooks will be executed. This case is different from the one implemented in #824 where we actually replace one logger for another.
I think we should explicitly return an error if the given logger is not of type *logrus.Logger and fail the test, until we fix #818.
Let me make the changes for this 👍
There was a problem hiding this comment.
Pushed a new version in d506083.
Now, the tests will fail also if the given logger is not of type *logrus.Logger, but in this case the behavior is less misleading.
Currently the tests don't fail because we ensure that the "initial" state.Logger is a *logrus.Logger when our implementation of log.New is called, or either create a new default one, as modified in #824 .
ka3de
force-pushed
the
fix/841-logcache-fieldlogger
branch
from
1100548
to
d506083
Compare
Replace the current panic usage when building our test browser implementation and use testing pkg Fatal/Fatalf methods instead. This avoids having to deal with panic stack trace during tests execution.
ka3de
force-pushed
the
fix/841-logcache-fieldlogger
branch
from
d506083
to
8740dd4
Compare
This commit fixes an incompatibility detected with k6 core usage of state.Logger, which has been modified in grafana/k6#2960. Co-authored-by: İnanç Gümüş <inanc.gumus@grafana.com>
ka3de
force-pushed
the
fix/841-logcache-fieldlogger
branch
from
8740dd4
to
1c23598
Compare
This PR fixes an incompatibility detected with k6 core usage of
state.Logger, which has been modified in grafana/k6#2960.This is done following the same approach as in #824 meanwhile a bigger refactor is applied, as defined in #818.
Closes #841 .