-
-
Notifications
You must be signed in to change notification settings - Fork 113
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
Bugfix/issue-140 broken record and verify tasks on windows #266
Bugfix/issue-140 broken record and verify tasks on windows #266
Conversation
…le path with name and wrong temp folder path
@pedrovgs Ready to check :) |
Hey @mariuszmarzec thank you so much for your contribution. I've merged a previous one that generates conflicts with yours because now the folder path is computed inside a data class improving a lot the quality of the code. Could you please update this PR? You can even move the OS param to the new |
…oken_record_on_windows # Conflicts: # core/src/main/scala/com/karumi/shot/Shot.scala # core/src/main/scala/com/karumi/shot/domain/model.scala # core/src/main/scala/com/karumi/shot/screenshots/ScreenshotsSaver.scala # core/src/test/scala/com/karumi/shot/ShotSpec.scala # core/src/test/scala/com/karumi/shot/domain/ConfigSpec.scala
@pedrovgs I changed slightly approach to fix this issue.
And second thing. After some thoughts i decided to not introduce dependency on system. Instead of i suggest to not use tmp system dir, we could use `build/tmp' path. |
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 code looks awesome now. Thank you so much @mariuszmarzec. I'm wondering if we could configure CI to check if the windows build is working automatically for us. I'm going to ask for a second check on the original issue too before merging the PR and releasing a new version 😃
@mariuszmarzec can you fix the code formatting? You can do it just running |
@pedrovgs Now should be fine, i tried |
@pedrovgs I have checked locally and only first one is failing on my machine and it is connected with keyboard. Seems to be not connected with changes.
Looks like using |
📌 References
🎩 What is the goal?
Verify and record tasks are broken on Windows due to hardcoded unix tmp dir and unix style metadata path returned by
Config.metadataFileName
method.How is it being implemented?
Replaced system tmp dir with tmp dir from build directory.
How can it be tested?
Added unit tests, tested locally on Windows
gradlew core:test