-
Notifications
You must be signed in to change notification settings - Fork 59
Add System.Formats.Tar test assets #238
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.
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
Conversation
| @@ -0,0 +1 @@ | |||
| Hello file No newline at end of file | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of "Hello" in each file, could this have a single-line description of what scenario the file represents?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I did it like this was so I could verify the expected file contents with a simple enough message that applied to all tests.
What do you think about putting a description of each test case in the readme instead?
|
|
||
| # Tar was executed elevated, need to ensure the | ||
| # generated archives are readable by current user | ||
| ResetOwnership $TargetDir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might consider adding some error handling in here to ensure the ownership is reset if something fails above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added instructions indicating that the script needs to be executed by a user with sudo permission.
Also, this script isn't expected to be executed often. But if it needs to be, then it would be very noticeable if something didn't go right by looking at the git status.
src/System.Formats.Tar.TestData/unarchived/many_small_files/9/9.txt
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1 @@ | |||
| Hello many_small_files | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any behaviors of the implementation where having behavior needs to be tested for larger files, different content types, newlines, non-ascii characters or other encoding scenarios, or anything else like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The contents of the file don't really matter much. The header stores the length in bytes of the data section, and the data section can contain anything in that space of the specified size.
But I can add tests (no need to add test assets) that verify that string fields do not have forbidden characters. For example, the Name field should not have an endline.
|
@jeffhandley @adamsitnik @jozkee can I get a new review? I addressed some of the comments and I prefer to not address a couple others. |
|
it may be beyond this but it may be nice to also have samples with:
may be hard/expensive but files bigger than MaxInt/MaxUint. |
|
Those are great suggestions, @wfurt. I can add those but I would like to do it in a separate PR so I don't keep blocking the main runtime PR. |
@jozkee @adamsitnik @jeffhandley @ericstj
Now that the tar API proposal has been approved, I would like to add some test assets, which are a pre-requisite for my main PR in dotnet/runtime.
This PR adds:
src/forSystem.Formats.Tar.TestData.*.tar(uncompressed) and*.tar.gz(compressed with Gzip) files that are also generated by the script, by collecting the unarchived files.The script generates files for all the supported formats:
tartool)tartool)These are the test data sets that are generated by archiving files under the unarchived folder (the tests that consume these tar/tar.gz files can compare the contents of the archive with the contents of the unarchived folder):
tartool seems to generate them all as "regular files".These are the test data sets that are generated by generating the necessary files on the fly, then archiving them, because either git or nuget do not support those filetypes (The tests that consume these tar/tar.gz files need to manually verify the expected contents, instead of comparing with the unarchived folder contents):