Skip to content

Use Dir instead of Base in DirectoryRefresher#21

Merged
rodaine merged 3 commits intomasterfrom
directory-fix
Jan 10, 2019
Merged

Use Dir instead of Base in DirectoryRefresher#21
rodaine merged 3 commits intomasterfrom
directory-fix

Conversation

@jameswang14
Copy link
Copy Markdown
Contributor

@jameswang14 jameswang14 commented Jan 10, 2019 •
edited
Loading

DirectoryRefresher is unable to pick up updates because the condition filepath.Base(path) == d.currDir effectively is never true. I think the intention was to use filepath.Dir here instead of filepath.Base (which fetches only the last part of the path)

Tests didn't catch this issue because of the malformed conditional. The tests only used file creates, which pass because of || op == Create

@jameswang14 jameswang14 reopened this Jan 10, 2019
@jameswang14
Copy link
Copy Markdown
Contributor Author

jameswang14 commented Jan 10, 2019

👀 @gitsen

@rodaine
Copy link
Copy Markdown
Contributor

rodaine commented Jan 10, 2019

Thanks for the patch! CI is failing, also there is apparently a test that covers the refreshing behavior. Can you see how the expectations there differ from yours? Could be the test is wrong.

@gitsen
Copy link
Copy Markdown
Contributor

gitsen commented Jan 10, 2019

Lgtm. Thanks for the fix.
We might need to update tests.

@jameswang14
Copy link
Copy Markdown
Contributor Author

jameswang14 commented Jan 10, 2019 •
edited
Loading

The test didn't catch this because it only does file creates, which would pass with the old implementation because the conditional wasn't grouped correctly:

filepath.Path(path) == d.currDir && op == Write || op == Create || op == Chmod

meant that Write events would require the first condition to also be true but Creates and Chmod would always pass.

I can update tests so it also includes a Write

@jameswang14
Copy link
Copy Markdown
Contributor Author

jameswang14 commented Jan 10, 2019 •
edited
Loading

@rodaine I closed and re-opened the PR to rerun tests. They seem to be passing on travis-ci/pr but seems like travis-ci/push didn't update? Is there a way to rerun that? They also pass locally FWIW

EDIT: never mind didn't realize the difference between /pr and /push, but confused why one would fail and the other wouldn't in this case since there haven't been any new commits to master?

rodaine
rodaine approved these changes Jan 10, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@rodaine rodaine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. The flakiness of these tests lead me to believe there's a race in there (though not from your patch). We'll want to investigate that further.

@rodaine rodaine merged commit c440f3f into master Jan 10, 2019
@rodaine rodaine deleted the directory-fix branch January 10, 2019 20:15
@ysawa0 ysawa0 mentioned this pull request Jun 11, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@rodaine rodaine rodaine approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jameswang14 @rodaine @gitsen