Test how the logger behaves in async mode by akerouanton · Pull Request #83 · fluent/fluent-logger-golang

akerouanton · 2020-05-07T06:12:00Z

New testcases have been written to test if PostWithTime() and Close()
are working as expected. More specifically, they test if Close() succeed
with all combination of options (Async + ForceStopAsyncSend).

Tests on PostWithTime() are divided into two parts:

With transient network failure ;
And without any ;

And tests on Close() are divided into two parts:

CloseOnAsyncConnect: When no logs have been written yet ;
CloseOnAsyncReconnect: When there're pending logs to write ;

Finally, the test Test_send_WritePendingToConn has been removed
because it was relying too much on the logger internals to test the
logger's behavior from an API user PoV. And the init() that was starting
a tcp listener has been removed to make the tests easier to execute
(otherwise, we've to wait for go test to timeout before re-running it).

akerouanton · 2020-05-07T06:13:45Z

@tagomoris This is a follow-up of #82, I prefer to send these test changes separately to ease the review. New test cases are marked as broken for now, but #82 is going to fix them 😉

akerouanton · 2020-05-20T16:53:39Z

Is there anything I could do to ease the review of this PR? 🙂

tagomoris · 2020-05-20T23:08:27Z

Ah, sorry for that I've not thought you'd finished the work on this (because a commit followed your comment).
I'll review this change later.

tagomoris

I've read through the entire diff and it looks good for now.
But let me check this once again later (tomorrow or next week?). It's complex and requires so much care.

tagomoris · 2020-05-21T08:43:24Z

fluent/fluent_test.go

+	case <-time.After(duration):
+		t.Fatalf("time out after %s: %s", duration.String(), reason)
+	case <-done:
+		return


Should we check case <-done before case <-time.After()?
(I'm not sure if the order of code affects the order of processes actually called)

By looking at Go spec (see here), I'd say the order doesn't matter since there're no default case and both receive cases here are blocking 😉

tagomoris · 2020-05-21T08:53:42Z

fluent/fluent_test.go

-			&TestMessage{Foo: "bar"},
-			"[\"tag_name\",1482493046,{\"foo\":\"bar\"},{}]",
+func TestCloseOnFailingAsyncReconnect(t *testing.T) {
+	t.Skip("Broken tests")


To be clear - With #82, we can remove this line, right?

I pushed a newer version of #82 with the commit from this PR and a reworked version of my initial fix. As you can see here, both t.Skip() are removed 🙂

I'll rebase #82 once this current PR got merged 😉

EDIT: btw, just saw the CI is failing on #82 but that's due to a bad rebase (he incriminated line shall have been removed). I'll fix that once this PR is merged to not rebase too many times as this is actually spamming the original issue on moby/moby repo.

tagomoris · 2020-05-27T09:00:20Z

fluent/fluent.go

+	if config.Timeout == 0 {
+		config.Timeout = defaultTimeout
+	}


Why not having all if config.ParameteName ... clauses about default values here, instead of newWithDialer?
Those configuration defaults are of Logger, and there seems no strong reason to do it in newWithDialer.
Having all the similar things in the same place should be valuable for readability.

Ah, code in test cases are assuming that defaults are set in newWithDialer. Hmm.

Indeed, I moved most of parameters initializations to newWithDialer() because I didn't want to refactor test cases too much. However, the timeout is needed only when the dialer is created by New().

tagomoris

This change looks good to me, but I have a concern about the difficulty newly introduced into testDialer and Conn. Now I understood those behaviors, but I can't be sure that I'll be able to work on those codes immediately when something is reported about it in the future.
That should be true for other people who will try to add new test cases.

The concern above could be unhelpful because this change wants to manage difficult things.
Improving naming (of writesCh, nextWriteCh ...) could be helpful, but I have no good idea about good names of them :(

So, I want to request to have short docs about how to use those test stubs, with sample code.
I'm ok either long comments in the test code file or an independent .md file.
It's best to have some sections about how to simulate

successful connection establishment, successful data transferring
successful connection establishment, failing data transferring
failing connection establishment
(and other possible scenarios)

@NiR- How do you feel about this idea?

akerouanton · 2020-07-25T14:16:26Z

@tagomoris Sorry for the long delay. I added ~120 lines of comments to better explain how things work, that include some examples, and explanations about the differences between test cases for sync and async modes.

About the namings, after revisiting this PR I wonder if following changes would make sense:

Replace waitForNewConn by waitForNextDialing (and connCh by dialCh) ;
Replace writeCh by writtenCh ;
And maybe nextWriteCh by nextWriteAttemptCh ;

These are small changes but maybe this would made things easier to understand. WDYT?

tagomoris · 2020-08-03T01:29:46Z

@akerouanton The added comments look really nice. Thanks!
I've found that the comments might be too long to have in the test case file, so I may be going to move it to an independent md document file later. Not sure now.

The proposed renames look also nice. 👍

New testcases have been written to test if PostWithTime() and Close() are working as expected. More specifically, they test if Close() succeed with all combination of options (Async + ForceStopAsyncSend). Tests on PostWithTime() are divided into two parts: * With transient network failure ; * And without any ; And tests on Close() are divided into two parts: * CloseOnAsyncConnect: When no logs have been written yet ; * CloseOnAsyncReconnect: When there're pending logs to write ; Finally, the test `Test_send_WritePendingToConn` has been removed because it was relying too much on the logger internals to test the logger's behavior from an API user PoV. And the init() that was starting a tcp listener has been removed to make the tests easier to execute (otherwise, we've to wait for go test to timeout before re-running it). Signed-off-by: Albin Kerouanton <albin.kerouanton@knplabs.com>

akerouanton · 2020-08-03T09:21:30Z

The proposed renames look also nice.

I just pushed these changes 😉

tagomoris · 2020-08-04T01:59:19Z

Merged. @akerouanton Thank you!
I'm looking forward to seeing changes on #82

CodePint · 2020-11-09T20:07:45Z

bump @tagomoris @akerouanton
see latest: moby/moby#40063

akerouanton force-pushed the improve-tests branch 4 times, most recently from 8361918 to e196026 Compare May 7, 2020 07:56

tagomoris self-requested a review May 20, 2020 23:08

tagomoris reviewed May 21, 2020

View reviewed changes

tagomoris reviewed May 27, 2020

View reviewed changes

tagomoris requested changes May 27, 2020

View reviewed changes

akerouanton force-pushed the improve-tests branch 2 times, most recently from 8fb0baa to 0465216 Compare July 25, 2020 14:14

akerouanton force-pushed the improve-tests branch from 0465216 to c19c097 Compare August 3, 2020 09:19

akerouanton mentioned this pull request Aug 3, 2020

Properly stop logger during (re)connect failure #82

Merged

tagomoris approved these changes Aug 4, 2020

View reviewed changes

tagomoris merged commit 89c0329 into fluent:master Aug 4, 2020

akerouanton deleted the improve-tests branch August 4, 2020 09:25

CodePint mentioned this pull request Nov 9, 2020

Container hangs when using fluentd logger moby/moby#40063

Closed

Conversation

akerouanton commented May 7, 2020

Uh oh!

akerouanton commented May 7, 2020

Uh oh!

akerouanton commented May 20, 2020

Uh oh!

tagomoris commented May 20, 2020

Uh oh!

tagomoris left a comment

Choose a reason for hiding this comment

Uh oh!

tagomoris May 21, 2020

Choose a reason for hiding this comment

Uh oh!

akerouanton May 21, 2020

Choose a reason for hiding this comment

Uh oh!

tagomoris May 21, 2020

Choose a reason for hiding this comment

Uh oh!

akerouanton May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tagomoris May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tagomoris May 27, 2020

Choose a reason for hiding this comment

Uh oh!

akerouanton Jul 25, 2020

Choose a reason for hiding this comment

Uh oh!

tagomoris left a comment

Choose a reason for hiding this comment

Uh oh!

akerouanton commented Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tagomoris commented Aug 3, 2020

Uh oh!

akerouanton commented Aug 3, 2020

Uh oh!

tagomoris commented Aug 4, 2020

Uh oh!

CodePint commented Nov 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

akerouanton May 21, 2020 •

edited

Loading

tagomoris May 27, 2020 •

edited

Loading

akerouanton commented Jul 25, 2020 •

edited

Loading