Limit log file name length to OS allowed limit #76
Conversation
Also change root for test_log_file_names tests to be a temporary directory to allow runnig tests without sudo. The old root `/` or `C:` is not usually writable for regular users.
pbos
left a comment
pbos
left a comment
There was a problem hiding this comment.
Sorry for the slow response time, new parent here.
| os.remove(log_name) | ||
| except OSError as os_error: | ||
| # truncate file name if error is Errno 36: File name too long | ||
| if os_error.errno == 36: |
There was a problem hiding this comment.
Can this be errno.ENAMETOOLONG instead of the comment?
There was a problem hiding this comment.
| Task._normalize(test_name), execution_number) | ||
| # The log file name is a combination of a test binary name and a test name. | ||
| # If the log file name exceeds OS limitations it will be truncated. | ||
| MKSTEMP_RANDOM_LEN = 12 # length of mkstemp random string being added |
There was a problem hiding this comment.
"# Space reserved for mkstemp suffix."
| # truncate file name if error is Errno 36: File name too long | ||
| if os_error.errno == 36: | ||
| prefix_length = ((os.statvfs('.').f_namemax - | ||
| MKSTEMP_RANDOM_LEN) if hasattr(os, 'statvfs') else MAX_PREFIX_LENGTH) - len(suffix) |
There was a problem hiding this comment.
I think this would read easier as: if hasattr(..): prefix_length = ..., i.e. avoid the ternary. as it's long.
| 'Test.case', 1)) | ||
| MAX_PREFIX_LENGTH = 240 | ||
| long_test_name = 'a' * (os.statvfs('.').f_namemax if hasattr(os, | ||
| 'statvfs') else MAX_PREFIX_LENGTH + 1) |
There was a problem hiding this comment.
Could you make this one a separate test? Maybe test_truncates_file_name
Also I think this test only makes sense on systems that have statvfs, can it run conditionally? This test requires OSes without statvfs to reject filenames that are longer which is maybe not what we want. I'm OK with not testing the MAX_PREFIX_LENGTH branch.
3 participants
No description provided.