*: Remove unnecessary configuration reload from ContentPathReloader and improve its tests
#6496
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,13 +57,15 @@ func TestPathContentReloader(t *testing.T) { | |
| testutil.Ok(t, pathContent.Rewrite([]byte("test modified again"))) | ||
| }, | ||
| }, | ||
| wantReloads: 2, | ||
| }, | ||
| { | ||
| name: "Chmod doesn't trigger reload", | ||
| args: args{ | ||
| runSteps: func(t *testing.T, testFile string, pathContent *staticPathContent) { | ||
| testutil.Ok(t, os.Chmod(testFile, 0777)) | ||
| testutil.Ok(t, os.Chmod(testFile, 0666)) | ||
| testutil.Ok(t, os.Chmod(testFile, 0777)) | ||
| }, | ||
| }, | ||
| wantReloads: 0, | ||
|
|
@@ -79,7 +81,9 @@ func TestPathContentReloader(t *testing.T) { | |
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| testFile := path.Join(t.TempDir(), "test") | ||
| testutil.Ok(t, os.WriteFile(testFile, []byte("test"), 0666)) | ||
| pathContent, err := NewStaticPathContent(testFile) | ||
|
|
@@ -98,6 +102,10 @@ func TestPathContentReloader(t *testing.T) { | |
| testutil.Ok(t, err) | ||
|
|
||
| tt.args.runSteps(t, testFile, pathContent) | ||
| if tt.wantReloads == 0 { | ||
| // Give things a little time to sync. The fs watcher events are heavily async and could be delayed. | ||
| time.Sleep(1 * time.Second) | ||
|
There was a problem hiding this comment. Crossposting from c6623f8#r120681785. Please check There was a problem hiding this comment. @GiedriusS I have some other follow ups to this part of the code and will include this suggested change for the next PR. 👍 There was a problem hiding this comment. But this loop will have a timeout though, as I don't want a test failure to end up with an infinite loop or rely on There was a problem hiding this comment. 👍 you can use runutil.Repeat with a context to achieve that There was a problem hiding this comment. @GiedriusS I actually have to keep the sleep there for one reason: a real scenario might have a delayed event from the fs and the test might pass if the assertion runs quickly before the event arrives... but would fail, as expected, if it had waited slightly more. I'm adding the repeat there though for all the other scenarios. |
||
| } | ||
| wg.Wait() | ||
| testutil.Equals(t, tt.wantReloads, reloadCount) | ||
| }) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this trigger 2 reloads now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one triggers zero reload... see line 71 below.
The one above it triggers 2 reloads, which is truly what we expect, because the file is rewritten twice. Previously it could pass due to the code being much faster than the async fs watcher notification for the second rewrite of the file.