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.

Sign up for GitHub

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

Remove date dependent screenshot path #1002

Closed
wants to merge 1 commit into from
Closed

Remove date dependent screenshot path #1002

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions airgun/session.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,8 @@ def take_screenshot(self):
session happens.
"""
now = datetime.now()
path = os.path.join(
settings.selenium.screenshots_path,
now.strftime('%Y-%m-%d'),
)
path = settings.selenium.screenshots_path

Copy link

@omkarkhatavkar omkarkhatavkar Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@jyejare, @ogajduse what are your thoughts?

Copy link
Collaborator Author

@dosas dosas Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@omkarkhatavkar omkarkhatavkar Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point I did not think of that

if not os.path.exists(path):
os.makedirs(path)
path = os.path.join(
Expand Down
Loading