lib/ignore: Centralize handling of temporary filenames (fixes #3899) #3901
Conversation
| // SetWindowsPrefix is entirely reserved for testing, it should never be called | ||
| // in production code. | ||
| // It is needed to allow static test files independent from underlying os. | ||
| func SetWindowsPrefix() { |
There was a problem hiding this comment.
Looks good to me except for this method which is ugly and clobbers any other usage of temp files after usage, in tests by other packages for example. Better alternatives imho are exporting the variables themselves, or a more general setprefixes method that can be called before and after tests to restor.
| @@ -25,9 +26,9 @@ func init() { | |||
| // We do this to make sure that the temp file required for the tests | |||
| // does not get removed during the tests. Also set the prefix so it's | |||
| // found correctly regardless of platform. | |||
| defTempNamer.prefix = windowsTempPrefix | |||
| ignore.SetWindowsPrefix() | |||
There was a problem hiding this comment.
I think we should use some reflect hack to access private methods or use the monkey patching library here.
I am not cool with exposing public API just for the sake of tests
| @@ -418,3 +418,19 @@ func IsInternal(file string) bool { | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| // ShouldIgnore returns true when a file is temporary, internal or ignored | |||
| func ShouldIgnore(filename string, matcher *Matcher) bool { | |||
There was a problem hiding this comment.
It feels that matcher should have a ShouldIgnore mehod instead which does Match and other stuff internally.
|
God damn github with it's new "oh I really want to comment" button that I keep forgetting to press. |
|
@AudriusButkevicius I was already wondering about your strangely long silence xD Regarding the testing uglyness: I can easily go ahead with calmh's proposition, but I agree with AudriusButkevicius: Ideally there should not be any exported function just for testing. Unfortunately I don't know the reflect hack or the monkey patching library, and google wasn't educating. Any hints? |
|
Or actuall, as @calmh suggests, just expose the variable. |
|
Monkey (in its current form) would be pointless, as it can neither replace internal stuff nor variables, only exposed functions. Regarding @calmh's comment
Are imported packages only loaded (i.e. |
|
They are called once per binary invocation, which happens once for all tests I suspect. So you need: originalPrefix = package.Prefix
package.Prefix = "X"
defer func() {
package.Prefix = originalPrefix
}() |
|
I rubberducked it and found TestMain (and didn't read Audrius' comment early enough), thus did it through that. Is it ok? |
|
@st-review lgtm |
|
@calmh: Noted! Need another LGTM or explicit merge command. |
|
@st-review lgtm |
st-review
added
the
pr-merged
st-review
added
the
frozen-due-to-age
For purpose see #3899, short recap:
Creating temporary names and testing whether a name is temporary is a "static" test, meaning it is set at
init()and never changed again. Passing it around from the models module is overly complicated and pointless. So this PR centralizes these functions in the ignore package for easy access for all libraries.I also refactored the function
shouldIgnorein models.go: The part that checks whether the filename is either internal, temporary or ignored is handled byShouldIgnorein the ignores package. This might seem unnecessary, as it is currently only used in models.go, but it contextually belongs there and such a function will be needed in fswatcher as well. Shouldn't hurt, right?The model tests (in specific rwfolder_test.go) use static test data with temporary windows prefixes. These were set in
init()of the test itself, which worked as the prefix is an internal variable of the same package. Now it is an internal variable of the ignore package. I resolved this by adding an exported function to the ignore package, that manually sets the prefix to the windows type. I considered this less bad than exporting the prefix itself.Is there any better way to both protect this variable in production and allow to set it for tests?