Skip to content
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

Exports file to temporary location if target is locked #416

Merged
merged 5 commits into from
Apr 12, 2017

Conversation

AmadeusW
Copy link
Contributor

@AmadeusW AmadeusW commented Apr 9, 2017

Fixes #414
When a target file exists and is locked, currently BDN crashes and some of the data is lost.
This PR makes BDN export data under randomly-unique name to a temporary location and print a message to the log:
image

// * Export *
Could not overwrite file C:\src\BenchmarkDotNet\samples\BenchmarkDotNet.Samples\BenchmarkDotNet.Artifacts\results\Algo_BitCount-report.csv. Exporting to C:\Users\amadeusz\AppData\Local\Temp\uvkf5nub-Algo_BitCount-report.csv
  C:\Users\amadeusz\AppData\Local\Temp\uvkf5nub-Algo_BitCount-report.csv
  BenchmarkDotNet.Artifacts\results\Algo_BitCount-report-github.md
  BenchmarkDotNet.Artifacts\results\Algo_BitCount-report.html

I'm not fully convinced that this sufficiently notifies user that the report in expected location is out of date, but I think this is better than data loss after a long running benchmark.

@dnfclas
Copy link

dnfclas commented Apr 9, 2017

@AmadeusW,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@adamsitnik
Copy link
Member

I like the idea, but I think that we should save the file to summary.ResultsDirectoryPath, not Path.GetTempPath().

Perhaps the name should be time-dependent by default? So our users could compare the results easily.

@AmadeusW
Copy link
Contributor Author

How about this? It yields a unique representation and follows ISO 8601

image

Is the message superfluous, and we could remove the second sentence?

// * Export *
Could not overwrite file C:\src\BenchmarkDotNet\samples\BenchmarkDotNet.Samples\BenchmarkDotNet.Artifacts\results\Algo_BitCount-report.csv. Exporting to C:\src\BenchmarkDotNet\samples\BenchmarkDotNet.Samples\BenchmarkDotNet.Artifacts\results\Algo_BitCount-report-20170409-192535.csv
  BenchmarkDotNet.Artifacts\results\Algo_BitCount-report-20170409-192535.csv
  BenchmarkDotNet.Artifacts\results\Algo_BitCount-report-github.md
  BenchmarkDotNet.Artifacts\results\Algo_BitCount-report.html

@adamsitnik
Copy link
Member

@AmadeusW I really like it!

@AndreyAkinshin what is your opinion on that?

@AndreyAkinshin
Copy link
Member

It's a common problem, so I really like that now we will have a solution for such cases.
@AmadeusW, thanks for the PR! Could you also write an integration test for it? It should be easy to write: just create *-report*.csv in advance, lock it (e.g. start to read from it), run a benchmark, check that we created this report with another name. I'm asking about it because we are going to implement other file creation issues (see a related issue: #377) and I want to be sure that future fixes will not break this one.

@AmadeusW
Copy link
Contributor Author

For sure

@adamsitnik adamsitnik merged commit fa31280 into dotnet:master Apr 12, 2017
@adamsitnik
Copy link
Member

@AmadeusW once again big thanks!

@AndreyAkinshin AndreyAkinshin added this to the v0.10.4 milestone Apr 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants