Fix flaky TestHotReloadUIConfigTempFile

[albertteoh]

Signed-off-by: albertteoh albert.teoh@logz.io

Which problem is this PR solving?

• Fixes Flaky test: TestHotReloadUIConfigTempFile #2711

Short description of the changes

• Flush file writes to disk to ensure subsequent reloads will pickup changes.
• Create a new temp dir just for temp files (purged upon completion) to ensure a clean test environment and avoid aforementioned watch error.
• Simplify and provide reusable file change wait logic based on approach taken in cert_watcher_test.go, using log messages as a signal.

Testing

• Integration tested locally with jaeger-query. Confirmed log message appearing due to missing UI config path.

{"level":"info","ts":1609828122.089528,"caller":"app/static_handler.go:170","msg":"UI config path not provided, config file will not be watched"}

• Ran 100 consecutive tests without error:

go clean -testcache && go test  -test.count=100 ./cmd/query/app -test.run TestHotReloadUIConfigTempFile

[codecov]

Codecov Report

Merging #2713 (50d73f4) into master (68acf65) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2713      +/-   ##
==========================================
+ Coverage   95.59%   95.72%   +0.12%     
==========================================
  Files         215      216       +1     
  Lines        9580     9588       +8     
==========================================
+ Hits         9158     9178      +20     
+ Misses        344      337       -7     
+ Partials       78       73       -5     

Impacted Files	Coverage Δ
cmd/query/app/static_handler.go	96.52% <100.00%> (+13.48%)	⬆️
pkg/fswatcher/fs_watcher.go	100.00% <100.00%> (ø)
cmd/query/app/server.go	88.52% <0.00%> (-1.64%)	⬇️
plugin/storage/integration/integration.go	77.34% <0.00%> (-0.56%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68acf65...50d73f4. Read the comment docs.

[albertteoh]

I don't understand why codecov thinks the fs_watcher.go file is untested, as TestHotReloadUIConfigTempFile will be executing these lines (my Goland IDE says this file is 100% covered).

Happy to add some simple tests instead of watcher/empty_test.go if need be.

[yurishkuro]

Are you sure this is necessary? Failure to instantiate a watcher can now prevent the service from starting.

[albertteoh]

I'm glad you asked as I don't understand how critical UI config watching is and leaned on the side of bubbling errors up.

If it's something that historically has been brittle (as I noticed from unit tests), I'd be happy to revert this back (it'd also simplify the PR); and errors are reported through logging anyway.

[albertteoh]

On the other hand, watching is only enabled (hence possible failure to start) if the user provides a UI configuration and, by default, the UI config path is "" which means watch() should just return a nil error. If the user provides UI configuration, they should expect the config to be watched and reloaded automatically. If it fails, I think it should prevent the service from starting, unless if we provide an opt-out flag.

If this error is encountered, the error message is quite clear and I believe, in most cases, is a straightforward resolution. e.g.:

{"level":"panic","ts":1609905512.493839,"caller":"app/static_handler.go:56","msg":"Could not create static assets handler","error":"cannot read UI config file /does/not/exist.json: open /does/not/exist.json: no such file or directory"

[yurishkuro]

In case of an obvious error like this I'd agree. But fsnotify/NewWatcher can return an error even without a file name, that error is likely to me much less sensible. Imaging collector is running on an architecture where fsnotify does not work, but the user wasn't interested in hot reload anyway, they just wanted to start with a custom config file. The old code would only log an error and continue working.

[albertteoh]

Okay, agree with this if existing users have an expectation of just running UI config without hot reloads. I'll revert back to returning nil.

[yurishkuro]

I don't understand why codecov thinks the fs_watcher.go file is untested

Perhaps because you exercise it from another package? Go tests to not extend coverage to other packages unless explicitly requested.

[albertteoh]

Perhaps because you exercise it from another package? Go tests to not extend coverage to other packages unless explicitly requested.

Yup, I did indeed exercise it from another package. I'll add some simple tests to cover this package as well.

[albertteoh]

I'm glad you asked as I don't understand how critical UI config watching is and leaned on the side of bubbling errors up.

If it's something that historically has been brittle (as I noticed from unit tests), I'd be happy to revert this back (it'd also simplify the PR); and errors are reported through logging anyway.

[yurishkuro]

Thanks, Albert!