Expanded test coverage for binary content#4332
Conversation
| // TODO - Add test coverage for binary diffs (if it isn't already elsewhere) | ||
|
|
||
| if actual.IsBinary { | ||
| if actual.contentWriter != nil { |
There was a problem hiding this comment.
It appears that for binary content, the actual content is always nil by default. 😕
There was a problem hiding this comment.
Yes, that's a correct. A git diff focuses on showing line-by-line changes, which is a concept that doesn't apply to binary files (like images, compiled code, or PDFs).
Instead of generating a meaningless content diff, Git simply records that the binary file has been modified. To access the actual raw content of the file, a different approach is needed. The standard command is git cat-file, which lets us stream the data directly for scanning here.
There was a problem hiding this comment.
Yeah, I read a bit about this that by default, git diff doesn’t render the content of binary files unless some configuration tweaks are made.
In that case I think these test cases are solid. We’re generating binary content and parsing it without any errors, which is essentially the goal here, right?
There was a problem hiding this comment.
❓ Does the git cat-file command only return binary gibberish, or does it provide human-readable output?
|
Correct me if I'm wrong: The test you modified doesn't actually parse the binary data, right? I can't find anywhere that your new, synthetic binary data is actually read in the test code, so I'm concerned that it might mislead readers. (I ran the updated test locally after fiddling with the test data and everything still passed.) |
Discussed locally. We need to check If we can find somewhere in the codebase that makes sense to test actual binary content. |
I reverted the previous changes in the |
|
I am planning to add some test cases to |
camgunz
left a comment
There was a problem hiding this comment.
LG! Love this approach 👍🏻
* Expanded test coverage for binary git diffs * reverted old changes and added binary test in chunker * revert more * added filesystem binary file scan test
Description:
This PR adds some test cases for git diff with binary content of different types.
Checklist:
make test-community)?make lintthis requires golangci-lint)?