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.

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

Add the possibility to embed screenshots in log.html as base64 content #332

Merged
merged 2 commits into from Oct 12, 2021

Conversation

arnaudruffin
Copy link
Contributor

@arnaudruffin arnaudruffin commented Oct 12, 2021

Here is a first step regarding issue #318

Implements

Add the possibility to set the parameter filename of capture_page_screenshot to "EMBED" in order to embed all screenshots directly as base64 content into the html log.

Fixing

#318

I have tested it in a real robotframework test project and it looks fine. If you are fine with that I think I could go a step further and handle a global parameter at library loading (as SeleniumLibrary is doing, have a look at https://github.com/robotframework/SeleniumLibrary/blob/master/src/SeleniumLibrary/keywords/screenshot.py)

@arnaudruffin arnaudruffin marked this pull request as ready for review October 12, 2021 09:02
@arnaudruffin
Copy link
Contributor Author

@serhatbolsu if you can have look, I can do more in the next few days if I have your consent... Getting back to it later may be more difficult for me :)

@serhatbolsu
Copy link
Owner

hi @arnaudruffin I like the feature, hardly used the screenshots on their own apart from the report. so it makes perfect sense to embed.

What do you think instead of giving a EMBED in the name, by default if no name is given, it just embeds. If user gives a name, then it writes to file?

@arnaudruffin
Copy link
Contributor Author

I like the idea @serhatbolsu , I found it "cleaner". But currently there is already a default behavior if no name is provided... don't you think it could disturb some users to have this behavior changed ?

@serhatbolsu
Copy link
Owner

I think this is a move forward from the current implementation. On the plus side it wouldnt be breaking change, if users realise that images are missing in files, they can simply give it a name. Documentation should clearly explain the behavior though

@arnaudruffin
Copy link
Contributor Author

Ok, I agree with you.

The other advantage of this is that I don't need to add an import parameter like "embed_all_screenshots", because it will be the new default behavior. I will do it right now.

…ven. Remove counter mecanism. Update documentation
@arnaudruffin
Copy link
Contributor Author

@serhatbolsu it looks ready for me. I tried to be clear in the documentation but feel free to change anything. I took the liberty to remove a paragraph mentioning a 'css' parameter that doesn't seem to exist.
I also specified that the behavior was new in "1.7", guessing a potential release name :)

@serhatbolsu
Copy link
Owner

did you test both ways, I am not in a position to test right now?

@arnaudruffin
Copy link
Contributor Author

I did this basic test:

Add the library in one of my project by copying the source.
Then I created the following robot tests:

*** Settings ***
Library         ../../Library/AppiumLibrary/

*** Test Cases ***
Let's take a screenshot
    AppiumLibrary.Open Application    [...]
    AppiumLibrary.Capture Page Screenshot

Let's take an embed screenshot
    AppiumLibrary.Open Application    [...]
    AppiumLibrary.Capture Page Screenshot   somefile

It worked fine (first screenshot was embedded, the second one was a link). But I'm in no hurry, take your time if you want to test that later

@serhatbolsu
Copy link
Owner

Thats great, thank you @arnaudruffin

@serhatbolsu serhatbolsu merged commit 5eba663 into serhatbolsu:master Oct 12, 2021
@arnaudruffin arnaudruffin deleted the feat-embed-screenshots branch October 12, 2021 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants