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.

Sign up for GitHub

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

Jump to bottom

[BUG] File change notifications behaviour is inconsistent (OS specific) #313

Closed
skyerus opened this issue Jan 24, 2023 · 0 comments · Fixed by #361
Closed

[BUG] File change notifications behaviour is inconsistent (OS specific) #313

skyerus opened this issue Jan 24, 2023 · 0 comments · Fixed by #361
Assignees
@skyerus
Labels
bug Something isn't working

Comments

@skyerus
Copy link
Contributor

skyerus commented Jan 24, 2023 •
edited
Loading

Observed behavior

While implementing integration tests of flagd, I initially ran the tests against the flagd binary which seemingly arbitrarily passed/failed. Conversely, the tests always pass while using the built flagd image directly.

The "flakey" tests are the ones that make a change to the flag config file, sleeps 50 milliseconds & then asserts that the flag has been cleared from cache.

Increasing the sleep (all the way up to 2 seconds) did nothing to fix the flakiness. This leads me to believe the behaviour of my mac's file change notifications system is inconsistent with the behaviour of flagd's distroless image's system.

Expected Behavior

The tests to pass as consistently when using the flagd binary as when using flagd's image.

Steps to reproduce

Not easily reproducible right now (the flagd integration tests and its dependencies are yet to be merged). Will update when they have been
@skyerus skyerus added the bug Something isn't working label Jan 24, 2023
@skyerus skyerus self-assigned this Jan 24, 2023
@skyerus skyerus mentioned this issue Jan 24, 2023
Merged
@skyerus skyerus mentioned this issue Feb 1, 2023
Merged
toddbaert added a commit that referenced this issue Feb 1, 2023
@skyerus @toddbaert
fix: use event.Has func for file change notification handling (increa…
09f74b9 
…sed stability across OS) (#361)

<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR

- Uses event.Has func for file change notification handling (instead of
direct comparison). This is the correct approach as [expressed directly
in fsnotify's
code](https://github.com/fsnotify/fsnotify/blob/main/fsnotify.go#L39).

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Fixes #313 

### Notes
<!-- any additional notes for this PR -->

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be
noted under this section -->
<!-- if there is a need for a new issue, please link it here -->

### How to test
<!-- if applicable, add testing instructions under this section -->

---------

Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Todd Baert <toddbaert@gmail.com>
Co-authored-by: Todd Baert <toddbaert@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@skyerus skyerus

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
1 participant
@skyerus