Embed screenshots in HTML formatter fails if src path is relative to the location of formatter

[aikhelis]

The "unless" condition here could be improved in distingushing whether src is a path to image or is an encoded data object

def embed(src, mime_type, label)
        case(mime_type)
        when /^image\/(png|gif|jpg|jpeg)/
          unless File.file?(src) or src =~ /^data:image\/(png|gif|jpg|jpeg);base64,/
            type = mime_type =~ /;base[0-9]+$/ ? mime_type : mime_type + ";base64"
            src = "data:" + type + "," + src
          end
          embed_image(src, label)

Problem:
In my case, src is a relative path to the screenshot file, so File.file?(src) ran in the context of the file where html formatter is defined - will return false. This leads to the following src of image in the html report. Thus, image is not found in the report.

src="data:image/png;base64,screenshots/my_screenshot.png"

[mattwynne]

@aikhelis can you help us to fix this by giving me a concrete example of how you use it? Where do the tests run? On CI? What are the paths that you're using?

[mattwynne]

Is this fixed by cucumber/common#697 ?

[brasmusson]

@aikhelis It is true that passing paths relative to the directory of the html report did work (and was more or less necessary unless using absolute paths), but it did have the drawback that activating the JSON formatter in addition to the HTML formatter would result in a failure. The JSON formatter always embed the actual data in the report, so paths passed in embed calls need to be readable from the location where formatters are executed.

Now that #697 is merged, the HTML formatter will convert paths passed in embed call, to be relative to the directory of the html report (if the the report is written to a file, that is). So now you should get the HTML report you want by passing paths relative to the location where formatters are executed (or absolute paths).

[tfcardoso]

Hi,

now with v2.0 my screenshots are in report with the extra ... discussed above. What I can do in my code in order to correct it?

def takeScreenshot
    filename = "#{Time.now.to_i}_screen.png"
    screenshot filename
    embed filename,'image/png'
end

[vveliev]

embed id far from perfect , is there any work that is planned to improve it ?!

[akostadinov]

Hello, let me chime in this issue as I have a similar concern. We are copying HTML reports to a server so using absolute path is not an option. Using relative path is also not a good option because the test suite component creating the file may not be aware of where formatter expects this path to be relative from.

Additionally, when multiple formatters are used simultaneously, they may expect the file to be relative to different base paths.

My proposal is to change HTML formatter to copy the file to an appropriate location at the time of embedding. This would allow one to embed temporary files that each formatter would handle as it sees fit, then the original embedded file can be removed (after the embed call returns).

An additional proposal I have is to change the RbWorld API to be:

def embed(data, mime_type, label='Screenshot')

instead of the current

def embed(file, mime_type, label='Screenshot')

This should encourage all formatters to handle raw data properly and put it in files or log or whatever as they see fit without the need to use an intermediate file when that makes sense. If HTML formatter supports non-file src, then probably Cucumber team already considers that as a valid use case?

And finally one issue with File.file?(src). This can raise an exception if src is raw image (or other type) data that contains unprintable characters. For example try putting a null byte in src. My solution was to just pass around base64 data and change my methods to do (File.file?(src) rescue false). I think though that most efficient will be for official formatters to also support random data when embedding and formatter to decide whether to put it into a file, inline it as base64 or something else. Of course embedding by file path is also fine as long as formatters understand the need to read/copy that file to an appropriate location before returning from the embed call.

P.S. the above are suggestions to make Cucumber API more solid. I'm not blocked on anything and I understand how to workaround undesired (according to my opinion) behavior.

[mattwynne]

@akostadinov I like this proposal. I wonder if, in order to avoid regressions from changing the API, we should make this a new method, perhaps called attach. WDYT?

cc @aslakhellesoy I think this would interest you.

[brasmusson]

I Cucumber-JVM the signature of the corresponding method (available only in hooks - but anyway) already:
void embed(byte[] data, String mimeType);,
so that "embed" signals a file name argument is not common everywhere.

[akostadinov]

Probably it is useful to keep supporting file paths as well data. For example some artifacts to embed might be generated outside ruby process and there is no need to load that potentially large data in memory.

Changing the parameter name from file to data or src should not introduce any regressions, it would just be some clue for people reading the API or the source code. Here my proposal does not imply any functional change.

The functional change I'm proposing is HTML formatter to:

• when file is provided, then copy it to report (or embed as Data URI) instead of just converting to relative path in the html
• when raw data with mime type is provided, then copy to file or embed as Data URI
• when Data URI is provided, then embed it or copy data to file

Also documenting what data should custom formatters be able to handle will also help.

I think this proposal should not introduce any regressions. This about File.file?(src) is just an implementation detail that I've hit so I thought to mention it here.

If Cucumber team is ok with the proposal, I can take a try at a pull request when I find some time. But I would like to first know your preferred behavior to reduce number of turnarounds.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.