Capture Page Screenshot should not overwrite if file already exist

[aaltat]

The #501 tries to fix the issue that if the file already exist, then index is added to the file name and file is created with index in file name. Example:

| Capture Page Screenshot | filename.png |
| Capture Page Screenshot | filename.png |
| File Should Exist | filename.png |
| File Should Exist | filename-1.png |

Making issue also, because it helps perform better tracking and if I screw the pull request again, we do not lose the conversation history.

[pekkaklarck]

Should this possibly be configurable with an optional parameter to avoid backwards incompatibility issues? Or should we support something like file-%d.png that tells S2L to replace ℅d with an index? It would also allow configuring where the index is added.

Regardless is the index configurable, it would be a good idea for the keyword to return an absolute path to the created image.

[aaltat]

There is overwrite=False parameter to control indexing and therefore the change is not backwards compatible. If we should set it to overwrite=True then it would work like the old implementation. This was briefly discussed in #426 but I also feel that we did not think the issue close enough.

I agree that returning the path is a good idea and I will make the change.

The %d is and interesting proposal. It also opens door to other formatting rules which could be nice to have (but perhaps in next commit and not in this one). With fast thinking, it should not be too hard to do.

So should this be default:

| Capture Page Screenshot | filename.png | 
| Capture Page Screenshot | filename.png |
| File Should Exist | filename.png |
| File Should Not Exist | filename-1.png |

In case of %d, should we support something like this

| Capture Page Screenshot | filename-%d.png | 
| Capture Page Screenshot | filename-%d.png |
| File Should Exist | filename-1.png |
| File Should Exist | filename-2.png |

In this scenario, should we also keep the overwrite argument.

[pekkaklarck]

I don't think both ℅d and overwrite would be needed. Nor do I know which would be better solution...

[aaltat]

My need is purely to avoid overwrites when I run my test with pabot. I simply want do define the filename with variable l, like ${BROWSER} and I would like avoid adding unique runing number in the end in the robot data.

The %d opens nice possibilities and could be more useful. Perhaps I should write a prototype and we can see in which way is the best.

[pekkaklarck]

+1 for prototypes. After thinking this a bit more, I start to like %d approach more and more. Making overwrite the default behavior would be more convenient in some cases but being backwards incompatible in others is a big minus.

[aaltat]

I am also starting to think that %d is best way to go. Also returning the absolute path is a good idea. I will write prototype and let's see how it does look.

[aaltat]

Made the proposal to support %d in #504

#504 proposal does not check does the file exist, but it also meets my needs. Personally I think the #504 is better and provides easier way extend the functionality in the future if needed. The only, although small, possibility for backwards incompatible is that someone might be already use %d in their filename.

[zephraph]

I like this much, much better. I completely agree with both of you, the %d approach looks excellent.

[aaltat]

I am thinking the same way and let's go with the %d option. I will test few corner cases, read the code again and at least write better tests.

One thing which comes in my mind, that the code is written now in way that it does not count the number of %d in the filename. If there are more than one, it will raise an exception. But I will give it some thoughts and let's see...

[zephraph]

I don't think it would make sense to allow more than one. Check for multiples, raise an exception, and document it.

[aaltat]

True, I will document it and write a test.

[aaltat]

I did updated documentation and re-write the tests in pull request #504. Review would be really nice.

[aaltat]

Did refactor the %d to {index} in #504 because it was causing more problems that it tried to solve. Also now it uses str.format in the underlying code and it solves the problems that the %d did have.

I am not totally sure, did I get the documentation totally in the way that would be correct, staring from here: https://github.com/robotframework/Selenium2Library/pull/504/files#diff-5346a546c6b37e3c016ad51e7155887dR55

Comments...

[zephraph]

The content of the documentation itself looks good. Formatting can always be tricky, but nothing jumps out at me. You should probably generate out the documentation from your branch and make sure it looks right before merging.

[aaltat]

Closed because #504 was merged.

[boakley]

I realize this is an old issue, but I think it might need to be revisited. There is a very bad default behavior which I think would be good to fix.

If I run a test, get some failures, then run robot again with --rerunfailed, the screenshots in my log become completely unreliable. While any re-ran tests will have good screenshots, all of the screenshots of the passing tests may be overwritten. It would be nice if the default behavior didn't cause this problem.

Here's a concrete example. Save this to /tmp/example.robot:

*** Settings ***
Library    Selenium2Library

Suite Setup     Open browser  http://example.com    chrome
Suite Teardown  close all browsers

*** Test Cases ***
Passing test
    go to   http://www.google.com
    capture page screenshot

Randomly failing test
    go to  http://www.bing.com
    capture page screenshot
    fail  bummer.

Run the following commands:

$ pybot /tmp/example.robot
$ pybot --output rerun.xml --rerunfailed output.xml /tmp/junk.robot
$ rebot --merge output.xml rerun.xml

When you do that, the screenshot in the first test will be of bing.com even though the test went to google.com and took a screenshot.

While this can be worked around by writing my own "capture page screenshot" keyword, it would be nice if the default behavior played nice with robot's -rerunfailed option out of the box.

[aaltat]

And there are other ways to run into same problem, example with pabot. Reopened because it is valid issue.

Would you like to submit PR to fix the problem?

[boakley]

Yeah, I can probably find time to make a pull request. My thought was to keep the current behavior when a filename is explicitly provided to "capture page screenshot" . It is only when a name is computed do we check for existence. Does that make sense? That way it should just work out of the box without requiring configuration or additional flags.

…

On Fri, May 26, 2017 at 10:47 AM, Tatu Aalto ***@***.***> wrote: And there are other ways to run into same problem, example with pabot. Reopened because it is valid issue. Would you like to submit PR to fix the problem? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/robotframework/Selenium2Library/issues/502#issuecomment-304317655>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEmYpvdaZXlxiVU2upL5dAHhCnWSW9Lks5r9vQpgaJpZM4F1-gR> .

[aaltat]

Does computed mean that the filename argument is with the default value or does it mean that filename can be given but contain the running index number? If it would be possible, I would prefer the later.

[boakley]

What I've got at the moment is that my new code only runs if the filename contains "{index}"*. If it does, then I compute the filename and then check if it exists. If it exists then I add one and try again. lather, rinse, repeat until we compute a filename that doesn't exist. I've tested this against a folder with 10,000 existing files (meaning, the first time it runs it calls os.path.exists 10,000 times) and it only takes a couple milliseconds or so on my linux box. I'm guessing it will be slower on windows, but probably not egregiously so. That means that if you have files with the number 1, 2, 5, 6, then run a new test (which causes the counter to reset to zero), the next screenshot will have 3, and the one after that will have 7, and then 8, then 9, ... * perhaps I should check for {index.*} in case someone tries to get fancy with the formatting.

…

On Fri, May 26, 2017 at 2:41 PM, Tatu Aalto ***@***.***> wrote: Does computed mean that the filename argument is with the default value or does it mean that filename can be given but contain the running index number? If it would be possible, I would prefer the later. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/robotframework/Selenium2Library/issues/502#issuecomment-304371753>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEmYgHiIxStNkpn8Yz5ZGJvgdwf2Bj7ks5r9yrXgaJpZM4F1-gR> .

[aaltat]

Sounds good to me, specially with the {index.*} formatting.