Fix #53 Unable to delete file on windows #306
Conversation
|
@pedrovgs It should be fine now. When I run the command locally, every scala file is reported as Files incorrectly formatted 馃 (even the ones I didn't edit) |
There was a problem hiding this comment.
Thanks for your contribution @badoualy 馃憦 Apart form the comments in the code, I'm going to try to answer your questions here:
I kept an empty suffix for the tests to avoid breaking everything. I'm not sure how you'd want to handle it. Maybe add new specific cases for this case?
We should add specific coverage yeap.
Small doubt about the System.currentTimeMillis().toString part, first time writing scala, not sure if the timestamp will be generated at each run, or once when the plugin is configured.
Why don't we replace this with a UUID? I think the timestamp is going to be generated for each run btw.
We should test these changes from a windows machine before merging. Remember there are commands you can use in order to install the artifact locally and test.
| FileUtils.deleteDirectory(new File(shotFolder.pulledComposeOrchestratedScreenshotsFolder())) | ||
| } catch { | ||
| case e: Throwable => println(Console.RED + s"Shot error: $e") | ||
| } |
There was a problem hiding this comment.
Instead of duplicating all these lines, can we extract common code to a function? I don't remember if there was a file to handle file management tasks, but at least we could use a private function for this.
| @@ -9,10 +9,12 @@ case class ShotFolder( | |||
| private val flavor: Option[String], | |||
| private val directorySuffix: Option[String], | |||
| private val separator: String, | |||
| private val orchestrated: Boolean | |||
| private val orchestrated: Boolean, | |||
| private val uid: String | |||
There was a problem hiding this comment.
Is this UUD something we really need to fix this bug? Or just the previous change is enough?
There was a problem hiding this comment.
The previous change will ensure that when the files are locked by the process, calling delete won't crash and make the task fail.
But since the files might not be deleted, attempting to re-run the task will fail because the file already exist, and can't be deleted.
I'll see what I can do
We can Edit: I tested, the try/catch part is ok, the directory suffix is NOK, looks like files are not renamed when copied from device, I'll attempt to fix when I have more time Edit2: |
|
@pedrovgs The UUID solution won't work, because the UUID will be different between the So we can keep the try/catch to avoid having the task fail and still generate the report. But the next run will still fail unless the file is deleted manually 馃し |
There was a problem hiding this comment.
@badoualy I'm approving and merging this PR. It will be included in the next release I'm about to publish. I'm wondering if there is something we can do to ensure we don't break windows support in the future. Different users reported in the past this library wasn't working for windows, others reported that it worked and some PRs were merged in order to fix this but looks like it wasn't always fully working. Can we do something to improve this situation and ensure we validate windows integration at least for the recording and verification tasks?
|
@pedrovgs what about GithubActions? Have no experience with it, but I see in this blog post that one could run tests on specific machines? |
馃搶 References
org.apache.commons.io.FileExistsException聽#244馃帺 What is the goal?
Attempt to fix issues with gradle process locking files on windows.
How is it being implemented?
removeProjectTemporalScreenshotsFoldercall belowgenerateVerificationReportto avoid crashing before the report is generateduidsuffix for the pulled screenshot foldersdeletecall in a try/catch to not crashHow can it be tested?
No particular step. But I didn't test yet, I need to try publishing it on my local maven repo or use jitpack.
Note
System.currentTimeMillis().toStringpart, first time writing scala, not sure if the timestamp will be generated at each run, or once when the plugin is configured.The PR is more of a draft/suggestion to help the author rather than a ready-to-merge PR.
If you have feedbacks I can try improving it.