Using Capture Screenshot with %d in filename #504
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| class _ScreenshotKeywords(KeywordGroup): | ||
|
|
||
| def __init__(self): | ||
| self._screenshot_index = {} | ||
| self._screenshot_path_stack = [] | ||
| self.screenshot_root_directory = None | ||
|
|
||
|
|
@@ -17,49 +17,86 @@ def __init__(self): | |
| def set_screenshot_directory(self, path, persist=False): | ||
| """Sets the root output directory for captured screenshots. | ||
|
|
||
| ``path`` argument specifies the absolute path where the screenshots | ||
| should be written to. If the specified ``path`` does not exist, | ||
| it will be created. Setting ``persist`` specifies that the given | ||
| ``path`` should be used for the rest of the test execution, otherwise | ||
| the path will be restored at the end of the currently executing scope. | ||
| """ | ||
| path = os.path.abspath(path) | ||
| self._create_directory(path) | ||
| if persist is False: | ||
| self._screenshot_path_stack.append(self.screenshot_root_directory) | ||
| # Restore after current scope ends | ||
| utils.events.on('scope_end', 'current', | ||
| self._restore_screenshot_directory) | ||
|
|
||
| self.screenshot_root_directory = path | ||
|
|
||
| def capture_page_screenshot(self, | ||
| filename='selenium-screenshot-{index}.png'): | ||
| """Takes a screenshot of the current page and embeds it into the log. | ||
|
|
||
| ``filename`` argument specifies the name of the file to write the | ||
| screenshot into. If no ``filename`` is given, the screenshot is saved | ||
| into file _selenium-screenshot-{index}.png_ under the directory where | ||
| the Robot Framework log file is written into. The ``filename`` is | ||
| also considered relative to the same directory, if it is not | ||
| given in absolute format. If an absolute or relative path is given | ||
| but the path does not exist it will be created. | ||
|
|
||
| Starting from Selenium2Library 1.8 if ``filename`` contains _{index}_ | ||
| characters, it will be automatically replaced with running index. | ||
| The running index is unique for each different filename. The absolute | ||
| path of the saved screenshot is always returned and it does not depend | ||
| does the ``filename`` contain _{index}_. See example 1 and 2 for more | ||
| details. | ||
|
|
||
| The _{index}_ is replaced with the actual index by using Python's | ||
| [https://docs.python.org/2/library/stdtypes.html#str.format| | ||
| str.format] method, and it can be formatted using the standard | ||
| [https://docs.python.org/2/library/string.html#format-string-syntax| | ||
| format string syntax]. The example 3 shows this by setting the width and | ||
| the fill character. | ||
|
|
||
| If there is a need to write literal _{index}_ or if ``filename`` | ||
| contains _{_ or _}_ characters, then the braces must be doubled. | ||
|
|
||
| Example 1: | ||
| | ${file1} = | Capture Page Screenshot | | ||
| | File Should Exist | ${OUTPUTDIR}${/}selenium-screenshot-1.png | | ||
| | Should Be Equal | ${file1} | ${OUTPUTDIR}${/}selenium-screenshot-1.png | | ||
| | ${file2} = | Capture Page Screenshot | | ||
| | File Should Exist | ${OUTPUTDIR}${/}selenium-screenshot-2.png | | ||
| | Should Be Equal | ${file2} | ${OUTPUTDIR}${/}selenium-screenshot-2.png | | ||
|
|
||
| Example 2: | ||
| | ${file1} = | Capture Page Screenshot | ${OTHER_DIR}${/}other-{index}-name.png | | ||
| | ${file2} = | Capture Page Screenshot | ${OTHER_DIR}${/}some-other-name-{index}.png | | ||
| | ${file3} = | Capture Page Screenshot | ${OTHER_DIR}${/}other-{index}-name.png | | ||
| | File Should Exist | ${OTHER_DIR}${/}other-1-name.png | | ||
| | Should Be Equal | ${file1} | ${OTHER_DIR}${/}other-1-name.png | | ||
| | File Should Exist | ${OTHER_DIR}${/}some-other-name-1.png | | ||
| | Should Be Equal | ${file2} | ${OTHER_DIR}${/}some-other-name-1.png | | ||
| | File Should Exist | ${OTHER_DIR}${/}other-2-name.png | | ||
| | Should Be Equal | ${file3} | ${OTHER_DIR}${/}other-2-name.png | | ||
|
|
||
| Example 3: | ||
| | Capture Page Screenshot | ${OTHER_DIR}${/}sc-{index:06}.png | | ||
| | File Should Exist | ${OTHER_DIR}${/}sc-000001.png | | ||
| """ | ||
| path, link = self._get_screenshot_paths(filename) | ||
| self._create_directory(path) | ||
| if hasattr(self._current_browser(), 'get_screenshot_as_file'): | ||
| if not self._current_browser().get_screenshot_as_file(path): | ||
| raise RuntimeError('Failed to save screenshot ' + link) | ||
|
Member
There was a problem hiding this comment. I commented above that unrelated PEP-8 changes typically should be submitted separately. Indentation fix to code that is anyway touched is definitely fine. I might even remove the unnecessary empty lines around this if/else block...
Contributor
There was a problem hiding this comment. I'd just leave this part as is.
Contributor
Author
There was a problem hiding this comment. I did change the filename -> link and in the same time fixed the PEP-8. I was thinking same as @pekkaklarck to remove the empty lines around if/else but was thinking that it should be done in separate commit or not. And then did not do ti.
Member
There was a problem hiding this comment. From reviewers point of view, removing unnecessary empty lines is typically fine because understanding the change takes practically no time. Same with indentation fixes, especially when done for the code touched anyway. |
||
| else: | ||
| if not self._current_browser().save_screenshot(path): | ||
| raise RuntimeError('Failed to save screenshot ' + link) | ||
| # Image is shown on its own row and thus prev row is closed on purpose | ||
| self._html('</td></tr><tr><td colspan="3"><a href="%s">' | ||
| '<img src="%s" width="800px"></a>' % (link, link)) | ||
| return path | ||
|
|
||
| # Private | ||
| def _create_directory(self, path): | ||
|
|
@@ -87,14 +124,17 @@ def _restore_screenshot_directory(self): | |
| self.screenshot_root_directory = self._screenshot_path_stack.pop() | ||
|
|
||
| def _get_screenshot_paths(self, filename): | ||
| filename = filename.format( | ||
| index=self._get_screenshot_index(filename)) | ||
| filename = filename.replace('/', os.sep) | ||
| screenshotdir = self._get_screenshot_directory() | ||
| logdir = self._get_log_dir() | ||
| path = os.path.join(screenshotdir, filename) | ||
| link = robot.utils.get_link_path(path, logdir) | ||
|
Member
There was a problem hiding this comment. This should actually be replaced with
Contributor
Author
There was a problem hiding this comment. It almost work identically. One of the test do fail, because, if the Should I leave it as it was, try to fix the error in this commit or change the to use
Member
There was a problem hiding this comment. Oh, I seriously thought these two functions would be identical, but on a closer look there seem to be also other small differences. Please keep it like it is. Due to differences it is unlikely that |
||
| return path, link | ||
|
|
||
| def _get_screenshot_index(self, filename): | ||
| if filename not in self._screenshot_index: | ||
| self._screenshot_index[filename] = 0 | ||
| self._screenshot_index[filename] += 1 | ||
| return self._screenshot_index[filename] | ||
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.
Are these changes here to avoid lines longer than 80 chars? That is OK in general, but such changes should be submitted separately. Preferably so that the whole module is gone through in one go. In minor cases it's also a bit questionable is the slightly enhanced PEP-8 compatibility worth an extra change in the version history.
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.
I mostly agree with what @pekkaklarck said. We really should keep changes focused. With that being said, I'm not necessarily against this staying. I'll defer to you two on that decision.
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.
Yes, modifications where done do void too long lines. I can can remove it from the commit if it's needed.
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.
No need to remove. Unrelated changes just add noise that makes the review a bit harder. Now we all already know why the changes are here and removing them wouldn't win much.