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

Update actions, remove use of deprecated package, and some refactor / fixes #113

Closed

Conversation

thaJeztah
Copy link

See individual commits for details:

gha: update actions/checkout@v3, actions/setup-go@v3

Older versions are deprecated, so updating to current versions.

all: gofmt code for current go versions

fix example in README

use string-literals to prevent having to escape quotes

This makes the code and examples slightly more readable.

tests: make sure to capture test-case in loop

TestNoPanicOnAsyncClose was not capturing the testcase, but running
with t.Parallel().

Also updated other tests to use the same approach, and renamed variables
for consistency.

remove use of deprecated github.com/bmizerany/assert package

The github.com/bmizerany/assert module has been deprecated and is no
longer maintained. In addition, the dependency brings various indirect
dependencies with it;

module github.com/fluent/fluent-logger-golang

go 1.19

require (
    github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869
    github.com/tinylib/msgp v1.1.6
)

require (
    github.com/kr/pretty v0.3.1 // indirect
    github.com/kr/text v0.2.0 // indirect
    github.com/philhofer/fwd v1.1.1 // indirect
    github.com/rogpeppe/go-internal v1.9.0 // indirect
)

The assertion package itself also looks to have some issues. For example,
it's not marked as a t.Helper(), and while it includes the location of
the error in the output, the failures are prefixed with the location of
the assertion itself, which is somewhat confusing:

=== RUN   Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided
    assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:275
    assert.go:62: !  Unexpected: <24224>
--- FAIL: Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided (0.00s)

=== RUN   Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified
    assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:306
    assert.go:62: !  Unexpected: <"unix">
--- FAIL: Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified (0.00s)

=== RUN   Test_New_itShouldUseConfigValuesFromArguments
    assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:327
    assert.go:62: !  Unexpected: <6666>
--- FAIL: Test_New_itShouldUseConfigValuesFromArguments (0.00s)

=== RUN   Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument
    assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:333
    assert.go:62: !  Unexpected: <true>
--- FAIL: Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument (0.00s)

=== CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
    assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:653
=== CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
    assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:655
=== CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
    assert.go:62: !  Unexpected: <&errors.errorString{s:"fluent#appendBuffer: Logger already closed"}>
=== CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
    assert.go:62: !  Unexpected: <<nil>>

While a good assertion library can help (for example by printing a rich
diff output if a struct does not match), a look at how it's used shows
that most cases are comparing primitive types (int, string, bool). This
patch swaps the library for a local assertEqual() utility. With this
patch, test-failures look like the example below:

=== RUN   Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided
    fluent_test.go:281: got: '24224', expected: '24224'
    fluent_test.go:282: got: '127.0.0.1', expected: '127.0.0.1'
    fluent_test.go:283: got: '3s', expected: '3s'
    fluent_test.go:284: got: '0s', expected: '0s'
    fluent_test.go:285: got: '8192', expected: '8192'
    fluent_test.go:286: got: 'tcp', expected: 'tcp'
    fluent_test.go:287: got: '', expected: ''
--- FAIL: Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided (0.00s)

=== RUN   Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified
    fluent_test.go:312: got: 'unix', expected: 'unix'
    fluent_test.go:313: got: '/tmp/fluent-logger-golang.sock', expected: '/tmp/fluent-logger-golang.sock'
--- FAIL: Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified (0.00s)

=== RUN   Test_New_itShouldUseConfigValuesFromArguments
    fluent_test.go:333: got: '6666', expected: '6666'
    fluent_test.go:334: got: 'foobarhost', expected: 'foobarhost'
--- FAIL: Test_New_itShouldUseConfigValuesFromArguments (0.00s)

=== RUN   Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument
    fluent_test.go:339: got: 'true', expected: 'true'
--- FAIL: Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument (0.00s)

=== RUN   TestJsonConfig
    fluent_test.go:441: got: '{FluentPort:8888 FluentHost:localhost FluentNetwork:tcp FluentSocketPath:/var/tmp/fluent.sock Timeout:3µs WriteTimeout:6µs BufferLimit:10 RetryWait:5 MaxRetry:3 MaxRetryWait:0 TagPrefix:fluent Async:false ForceStopAsyncSend:false AsyncResultCallback:<nil> AsyncConnect:false MarshalAsJSON:true AsyncReconnectInterval:0 SubSecondPrecision:false RequestAck:false TlsInsecureSkipVerify:false}', expected: '{FluentPort:8888 FluentHost:localhost FluentNetwork:tcp FluentSocketPath:/var/tmp/fluent.sock Timeout:3µs WriteTimeout:6µs BufferLimit:10 RetryWait:5 MaxRetry:3 MaxRetryWait:0 TagPrefix:fluent Async:false ForceStopAsyncSend:false AsyncResultCallback:<nil> AsyncConnect:false MarshalAsJSON:true AsyncReconnectInterval:0 SubSecondPrecision:false RequestAck:false TlsInsecureSkipVerify:false}'
--- FAIL: TestJsonConfig (0.00s)

=== CONT  TestPostWithTime/without_Async
    fluent_test.go:177: got: '["acme.tag_name",1482493046,{"foo":"bar"},{}]', expected: '["acme.tag_name",1482493046,{"foo":"bar"},{}]'
    fluent_test.go:177: got: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]'
    fluent_test.go:177: got: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]', expected: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]'
=== CONT  TestPostWithTime/with_Async
    fluent_test.go:177: got: '["acme.tag_name",1482493046,{"foo":"bar"},{}]', expected: '["acme.tag_name",1482493046,{"foo":"bar"},{}]'
    fluent_test.go:177: got: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]'
    fluent_test.go:177: got: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]', expected: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]'

=== CONT  TestReconnectAndResendAfterTransientFailure/with_Async
    fluent_test.go:177: got: '["tag_name",1482493046,{"foo":"bar"},{}]', expected: '["tag_name",1482493046,{"foo":"bar"},{}]'
=== CONT  TestReconnectAndResendAfterTransientFailure/without_Async
    fluent_test.go:177: got: '["tag_name",1482493046,{"foo":"bar"},{}]', expected: '["tag_name",1482493046,{"foo":"bar"},{}]'
    fluent_test.go:177: got: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]'
=== CONT  TestReconnectAndResendAfterTransientFailure/with_Async
    fluent_test.go:177: got: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]'

=== CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
    fluent_test.go:657: got: 'fluent#appendBuffer: Logger already closed', expected: 'fluent#appendBuffer: Logger already closed'
=== CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
    fluent_test.go:659: got: '<nil>', expected: '<nil>'

The list of dependencies have also been reduced with this patch:

module github.com/fluent/fluent-logger-golang

go 1.19

require github.com/tinylib/msgp v1.1.6

require github.com/philhofer/fwd v1.1.1 // indirect

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
This makes the code and examples slightly more readable.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
TestNoPanicOnAsyncClose was not capturing the testcase, but running
with t.Parallel().

Also updated other tests to use the same approach, and renamed variables
for consistency.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Author

@tagomoris (👋 it's been a while 😅 - hope your'e doing well) PTAL 🤗

@thaJeztah
Copy link
Author

Follow-up is in #114

@thaJeztah
Copy link
Author

@fujimotos PTAL 🤗

The github.com/bmizerany/assert module has been deprecated and is no
longer maintained. In addition, the dependency brings various indirect
dependencies with it;

    module github.com/fluent/fluent-logger-golang

    go 1.19

    require (
        github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869
        github.com/tinylib/msgp v1.1.6
    )

    require (
        github.com/kr/pretty v0.3.1 // indirect
        github.com/kr/text v0.2.0 // indirect
        github.com/philhofer/fwd v1.1.1 // indirect
        github.com/rogpeppe/go-internal v1.9.0 // indirect
    )

The assertion package itself also looks to have some issues. For example,
it's not marked as a `t.Helper()`, and while it includes the location of
the error in the output, the failures are prefixed with the location of
the assertion itself, which is somewhat confusing:

    === RUN   Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:275
        assert.go:62: !  Unexpected: <24224>
    --- FAIL: Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided (0.00s)

    === RUN   Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:306
        assert.go:62: !  Unexpected: <"unix">
    --- FAIL: Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified (0.00s)

    === RUN   Test_New_itShouldUseConfigValuesFromArguments
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:327
        assert.go:62: !  Unexpected: <6666>
    --- FAIL: Test_New_itShouldUseConfigValuesFromArguments (0.00s)

    === RUN   Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:333
        assert.go:62: !  Unexpected: <true>
    --- FAIL: Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument (0.00s)

    === CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:653
    === CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
        assert.go:15: /Users/thajeztah/go/src/github.com/fluent/fluent-logger-golang/fluent/fluent_test.go:655
    === CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
        assert.go:62: !  Unexpected: <&errors.errorString{s:"fluent#appendBuffer: Logger already closed"}>
    === CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
        assert.go:62: !  Unexpected: <<nil>>

While a good assertion library can help (for example by printing a rich
diff output if a struct does not match), a look at how it's used shows
that most cases are comparing primitive types (int, string, bool). This
patch swaps the library for a local `assertEqual()` utility. With this
patch, test-failures look like the example below:

    === RUN   Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided
        fluent_test.go:281: got: '24224', expected: '24224'
        fluent_test.go:282: got: '127.0.0.1', expected: '127.0.0.1'
        fluent_test.go:283: got: '3s', expected: '3s'
        fluent_test.go:284: got: '0s', expected: '0s'
        fluent_test.go:285: got: '8192', expected: '8192'
        fluent_test.go:286: got: 'tcp', expected: 'tcp'
        fluent_test.go:287: got: '', expected: ''
    --- FAIL: Test_New_itShouldUseDefaultConfigValuesIfNoOtherProvided (0.00s)

    === RUN   Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified
        fluent_test.go:312: got: 'unix', expected: 'unix'
        fluent_test.go:313: got: '/tmp/fluent-logger-golang.sock', expected: '/tmp/fluent-logger-golang.sock'
    --- FAIL: Test_New_itShouldUseUnixDomainSocketIfUnixSocketSpecified (0.00s)

    === RUN   Test_New_itShouldUseConfigValuesFromArguments
        fluent_test.go:333: got: '6666', expected: '6666'
        fluent_test.go:334: got: 'foobarhost', expected: 'foobarhost'
    --- FAIL: Test_New_itShouldUseConfigValuesFromArguments (0.00s)

    === RUN   Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument
        fluent_test.go:339: got: 'true', expected: 'true'
    --- FAIL: Test_New_itShouldUseConfigValuesFromMashalAsJSONArgument (0.00s)

    === RUN   TestJsonConfig
        fluent_test.go:441: got: '{FluentPort:8888 FluentHost:localhost FluentNetwork:tcp FluentSocketPath:/var/tmp/fluent.sock Timeout:3µs WriteTimeout:6µs BufferLimit:10 RetryWait:5 MaxRetry:3 MaxRetryWait:0 TagPrefix:fluent Async:false ForceStopAsyncSend:false AsyncResultCallback:<nil> AsyncConnect:false MarshalAsJSON:true AsyncReconnectInterval:0 SubSecondPrecision:false RequestAck:false TlsInsecureSkipVerify:false}', expected: '{FluentPort:8888 FluentHost:localhost FluentNetwork:tcp FluentSocketPath:/var/tmp/fluent.sock Timeout:3µs WriteTimeout:6µs BufferLimit:10 RetryWait:5 MaxRetry:3 MaxRetryWait:0 TagPrefix:fluent Async:false ForceStopAsyncSend:false AsyncResultCallback:<nil> AsyncConnect:false MarshalAsJSON:true AsyncReconnectInterval:0 SubSecondPrecision:false RequestAck:false TlsInsecureSkipVerify:false}'
    --- FAIL: TestJsonConfig (0.00s)

    === CONT  TestPostWithTime/without_Async
        fluent_test.go:177: got: '["acme.tag_name",1482493046,{"foo":"bar"},{}]', expected: '["acme.tag_name",1482493046,{"foo":"bar"},{}]'
        fluent_test.go:177: got: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]'
        fluent_test.go:177: got: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]', expected: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]'
    === CONT  TestPostWithTime/with_Async
        fluent_test.go:177: got: '["acme.tag_name",1482493046,{"foo":"bar"},{}]', expected: '["acme.tag_name",1482493046,{"foo":"bar"},{}]'
        fluent_test.go:177: got: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["acme.tag_name",1482493050,{"fluentd":"is awesome"},{}]'
        fluent_test.go:177: got: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]', expected: '["acme.tag_name",1634263200,{"welcome":"to use"},{}]'

    === CONT  TestReconnectAndResendAfterTransientFailure/with_Async
        fluent_test.go:177: got: '["tag_name",1482493046,{"foo":"bar"},{}]', expected: '["tag_name",1482493046,{"foo":"bar"},{}]'
    === CONT  TestReconnectAndResendAfterTransientFailure/without_Async
        fluent_test.go:177: got: '["tag_name",1482493046,{"foo":"bar"},{}]', expected: '["tag_name",1482493046,{"foo":"bar"},{}]'
        fluent_test.go:177: got: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]'
    === CONT  TestReconnectAndResendAfterTransientFailure/with_Async
        fluent_test.go:177: got: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]', expected: '["tag_name",1482493050,{"fluentd":"is awesome"},{}]'

    === CONT  TestNoPanicOnAsyncClose/Channel_closed_before_write
        fluent_test.go:657: got: 'fluent#appendBuffer: Logger already closed', expected: 'fluent#appendBuffer: Logger already closed'
    === CONT  TestNoPanicOnAsyncClose/Channel_not_closed_at_all
        fluent_test.go:659: got: '<nil>', expected: '<nil>'

The list of dependencies have also been reduced with this patch:

    module github.com/fluent/fluent-logger-golang

    go 1.19

    require github.com/tinylib/msgp v1.1.6

    require github.com/philhofer/fwd v1.1.1 // indirect

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the update_gha_and_some_refactoring branch from 524a54c to d2ecf21 Compare July 1, 2023 13:57
@thaJeztah
Copy link
Author

Okay, looks like go1.13 is no longer a real option in CI, as it now tries
to fetch versions of dependencies that no longer can work on that version;

# golang.org/x/tools/internal/gocommand
Error: ../../../go/src/golang.org/x/tools/internal/gocommand/invoke.go:399:62: undefined: os.ErrProcessDone
Error: Process completed with exit code 2.

See test-run on my fork; https://github.com/thaJeztah/fluent-logger-golang/actions/runs/5431400323/jobs/9877747186

@thaJeztah
Copy link
Author

closing this one in favour of #114

@thaJeztah thaJeztah closed this Jul 1, 2023
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.

1 participant