-
Notifications
You must be signed in to change notification settings - Fork 331
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
Fixed TRX file overwrite in certain circumstances #2508
Conversation
Hello @Haplois! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
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 would avoid using pessimistic concurrency, and instead bump the timestamp when we get a conflict. Maybe pass the trx filename as an object / strategy that can easily regenerate it, or if that would change the public APIs just use our knowledge of the format (as you already do) and bump the timestamp and retry.
src/Microsoft.TestPlatform.Extensions.TrxLogger/Resources/xlf/TrxResource.tr.xlf
Outdated
Show resolved
Hide resolved
src/Microsoft.TestPlatform.Extensions.TrxLogger/Utility/FileHelper.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.TestPlatform.Extensions.TrxLogger/Utility/FileHelper.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.TestPlatform.Extensions.TrxLogger/Utility/FileHelper.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs
Outdated
Show resolved
Hide resolved
I did have a look, as you requested, and it still seems very complex for what is basically a retry operation, but maybe I am not grasping the details. I think it might get easier if we passed in an object that contains the raw data that we used to generated the filename and provides a method to get Next, and it also indicates whether or not we should be rewriting because based on whether the user provided the name or not. At least that is how I understand from you this is determined. This would avoid the timestamp parsing code, and it looks like all the appropriate methods are internal so we should not have problem with changing public APIs. Let's talk about this a bit more during our sync. |
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs
Outdated
Show resolved
Hide resolved
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs
Show resolved
Hide resolved
Added the reason why we need to keep a specific pattern in TRX file naming.
Removed unnecessary whitespace.
test/Microsoft.TestPlatform.Extensions.TrxLogger.UnitTests/TrxLoggerTests.cs
Outdated
Show resolved
Hide resolved
…aplois/vstest into fix/dotnet-test-overwrite-2489
Description
Checks the target file name for saving the TRX logs and insures it is distinct. This check does not occur if a filename is implicitly set, in this case file will be overwritten and a warning is shown.
Related issue
Fixes #2489