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

github.com/sirupsen/logrus usage refactoring #2960

Merged
merged 1 commit into from
Mar 31, 2023
Merged

Conversation

na--
Copy link
Member

@na-- na-- commented Mar 7, 2023

For some reason, my very sleep-deprived brain got stuck on these logging minutia today 🤦‍♂️ Still, the zombie state somewhat good at slogging through repetitive code modifications, so at least something semi-useful might come out of it in the long run... 😅 🤞

This is one small part of #2958. Because of grafana/xk6-browser#818, this PR requires #2994 to be merged first.

The important parts here are replacement of the *logrus.Logger concrete type usage with logrus.FieldLogger interface in a bunch of places, most importantly in the the VU lib.State and the lib.TestPreInitState 🎉 The testutils refactoring from manually constructed objects to constructor functions + removing any usage of github.com/sirupsen/logrus/hooks/test is both making it easier to write tests now, and hopefully also making it easier to remove logrus in the future.

@inancgumus
Copy link
Member

Hey, @na--, I'll start working on grafana/xk6-browser#818.

In the first step, I switched to this branch, ran the tests at the root with the following command, and some failed. Are these known to be flaky?

$ go test ./... -v
...
=== RUN   TestTracer/Test_#2
    tracer_test.go:169: 
                Error Trace:    /Users/inanc/grafana/k6/lib/netext/httpext/tracer_test.go:169
                Error:          Should be true
                Test:           TestTracer/Test_#2
                Messages:       http_req_blocked is <= 0
--- FAIL: TestTracer (0.00s)
    --- PASS: TestTracer/Test_#0 (0.00s)
    --- PASS: TestTracer/Test_#1 (0.00s)
    --- FAIL: TestTracer/Test_#2 (0.00s)
--- FAIL: TestVUIntegrationClientCerts (0.00s)
    --- FAIL: TestVUIntegrationClientCerts/VerifyServerCert (0.00s)
        --- FAIL: TestVUIntegrationClientCerts/VerifyServerCert/Archive (0.00s)
        --- FAIL: TestVUIntegrationClientCerts/VerifyServerCert/Source (0.01s)
    --- PASS: TestVUIntegrationClientCerts/WithCert (0.00s)
        --- PASS: TestVUIntegrationClientCerts/WithCert/Archive (0.00s)
        --- PASS: TestVUIntegrationClientCerts/WithCert/Source (0.00s)
    --- PASS: TestVUIntegrationClientCerts/WithoutDomains (0.00s)
        --- PASS: TestVUIntegrationClientCerts/WithoutDomains/Source (0.00s)
        --- PASS: TestVUIntegrationClientCerts/WithoutDomains/Archive (0.00s)
    --- PASS: TestVUIntegrationClientCerts/WithoutCert (0.00s)
        --- PASS: TestVUIntegrationClientCerts/WithoutCert/Source (0.00s)
        --- PASS: TestVUIntegrationClientCerts/WithoutCert/Archive (0.00s)

@na-- na-- mentioned this pull request Mar 8, 2023
@na--
Copy link
Member Author

na-- commented Mar 8, 2023

It seems it was flaky and was disabled on Windows before (#902), but it's currently not disabled 😕 In any case, I added a note about it in #2144 (comment)

@na-- na-- requested review from codebien and imiric and removed request for codebien March 22, 2023 14:41
cmd/tests/test_state.go Show resolved Hide resolved
lib/vu_state.go Outdated Show resolved Hide resolved
lib/testutils/test_output.go Outdated Show resolved Hide resolved
@codebien
Copy link
Collaborator

codebien commented Mar 22, 2023

@na-- LGTM, I've mostly requested the change for the browser

@na-- na-- changed the base branch from master to update-xk6-browser March 31, 2023 07:09
@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

❗ No coverage uploaded for pull request base (update-xk6-browser@084f4ac). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head be6322c differs from pull request most recent head 30b57e7. Consider uploading reports for the commit 30b57e7 to get more accurate results

@@                  Coverage Diff                  @@
##             update-xk6-browser    #2960   +/-   ##
=====================================================
  Coverage                      ?   76.91%           
=====================================================
  Files                         ?      228           
  Lines                         ?    17043           
  Branches                      ?        0           
=====================================================
  Hits                          ?    13108           
  Misses                        ?     3089           
  Partials                      ?      846           
Flag Coverage Δ
ubuntu 76.91% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@na--
Copy link
Member Author

na-- commented Mar 31, 2023

This should now be ready for another review. I rebased on top of #2994, with some extra improvements.

There is one part of the codebase that I haven't refactored and instead used a type assertion hack to work around. It would require a much larger refactoring, since it has a bunch of problems with it. I opened #2968 specifically for that, so we can fix it in another PR

@na-- na-- added this to the v0.44.0 milestone Mar 31, 2023
@na-- na-- requested a review from mstoykov March 31, 2023 07:58
Base automatically changed from update-xk6-browser to master March 31, 2023 08:10
codebien
codebien previously approved these changes Mar 31, 2023
Copy link
Collaborator

@codebien codebien left a comment

Choose a reason for hiding this comment

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

LGTM

@na-- na-- marked this pull request as ready for review March 31, 2023 08:12
codebien
codebien previously approved these changes Mar 31, 2023
imiric
imiric previously approved these changes Mar 31, 2023
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Nice 👍

We'll still need some tedious work to switch to slog, as the interfaces don't quite match up, but this is a step in the right direction.

lib/testutils/logrus_hook.go Show resolved Hide resolved
lib/testutils/logrus_hook.go Outdated Show resolved Hide resolved
This is in preparation for removing that dependency at some future point
@na-- na-- dismissed stale reviews from imiric and codebien via f577879 March 31, 2023 10:14
@na-- na-- merged commit 9a635a5 into master Mar 31, 2023
@na-- na-- deleted the logrus-refactor branch March 31, 2023 10:36
ka3de added a commit to grafana/xk6-browser that referenced this pull request Apr 3, 2023
This commit fixes an incompatibility detected with k6 core usage of
state.Logger, which has been modified in grafana/k6#2960.
ka3de added a commit to grafana/xk6-browser that referenced this pull request Apr 3, 2023
This commit fixes an incompatibility detected with k6 core usage of
state.Logger, which has been modified in grafana/k6#2960.
ka3de added a commit to grafana/xk6-browser that referenced this pull request Apr 4, 2023
This commit fixes an incompatibility detected with k6 core usage of
state.Logger, which has been modified in grafana/k6#2960.
ka3de added a commit to grafana/xk6-browser that referenced this pull request Apr 4, 2023
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>
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.

None yet

5 participants