Remove date dependent screenshot path #1002
Conversation
* Do not implicitly overwrite settings.selenium.screenshots_path * Date is already set in the filename * Reduce datetime dependency
dosas
changed the title
|
Can one of the admins verify this patch? |
Gauravtalreja1
added
6.12.z
CherryPick
|
Are there any actions left required from me? |
I can not see any. Let me run our PR tester, that should show us if something broke on our end. |
|
trigger: test-robottelo |
|
PRT did not trigger a job build in our internal infra. @omkarkhatavkar Can you please take a look at what might be wrong? |
| now.strftime('%Y-%m-%d'), | ||
| ) | ||
| path = settings.selenium.screenshots_path | ||
|
|
There was a problem hiding this comment.
@dosas Thanks for creating the PR, I appreciate the work you added. Looking at the function doc string this was added long back, looking at this statement
All directories will be created if they don't exist. Make sure that the user running Robottelo has the right permissions to create files and directories. this could be the reason this was added. I see some value in that. Also, I see some benefits as follows
- It dynamically creates directories based on the current date, ensuring that the screenshots or files are organized in a structured manner. Imagine if someone specifies the '/tmp' path here; all the screenshots would be jumbled together in a single directory. It's much better to have them organized by date for better isolation.
There was a problem hiding this comment.
There is an easy fix for that don't use /tmp but use /tmp/screenshots (I assume this is why robottelo needs the permissions).
Don't take away all responsibility from the user.
I would even go as far and not put the date information into the filename, since this is duplicate information that is already provided by the filesystem.
There was a problem hiding this comment.
I would even go as far and not put the date information into the filename, since this is duplicate information that is already provided by the filesystem.
I have a different perspective on this. If we don't include the date and time in the screenshot filenames using (DateTime) f'{self.name}-screenshot-YYYY-mm-dd_HH_MM_SS.png', there's a risk of overwriting the same screenshots when running the test again.
There was a problem hiding this comment.
Good point I did not think of that
There was a problem hiding this comment.
I have reviewed the proposed changes in this pull request in light of the current requirements within Airgun and Robottelo, as indicated in my previous review comments https://github.com/SatelliteQE/airgun/pull/1002/files#r1371209526. After careful consideration, I find that these changes do not align with the current project requirements and standards.
Therefore, I regret to inform you that I am disapproving of this PR at this time. If you have any questions or require further clarification regarding the decision, please don't hesitate to reach out, and we can discuss how to best proceed.
settings.selenium.screenshots_pathSee also #989