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

Gauge should create screenshot and share the file details with plugin instead of sharing the base64 encoded screenshot. #1476

Closed
negiDharmendra opened this issue Aug 30, 2019 · 15 comments
Assignees

Comments

@negiDharmendra
Copy link
Contributor

Sending base64 encoded screenshot data with the SuiteExecutionResult increases the proto message size which causes #1239, getgauge/html-report#218 especially when there are a lot of screenshots in the SuiteExecutionResult.

To mitigate all above-mentioned issues we thought of streaming ExecutionResult to the plugins as soon as execution starts. It seems to be the right thing to do but then the plugins will have to own the responsibility of accumulating the result.

There is another approach by whihc we can mitigate above-mentioned issues.
Instead of shareing the base64 encoded screenshot Gauge can save the screenshot under the reports directory and can send the files path to plugin.

@negiDharmendra negiDharmendra self-assigned this Aug 30, 2019
@negiDharmendra negiDharmendra removed their assignment Sep 4, 2019
@BugDiver BugDiver self-assigned this Sep 5, 2019
@BugDiver
Copy link
Member

BugDiver commented Sep 5, 2019

To write the screenshots to file and share details, we discussed the following two approaches

  1. Gauge write screenshots to a location which is specified by the reporting plugin.

    Pros
    • Plugins get pre-generated screenshots and can include them without doing anything
    Cons
    • Gauge has to write screenshots to multiple places in case multiple plugins need the screenshots
    • Some configuration is required to tell the dir info to gauge
    • Multiple writes can slow the execution
  2. Gauge write screenshots in project dir and plugins copy them (if they need).

    Pros
    • Gauge does not need to write to multiple locations (less execution time)
    • No configuration required
    Cons
    • Can not delete the screenshots, it would be required for regenerate feature.
    • One extra copy of the screenshot
    • Extra work for plugin

@BugDiver
Copy link
Member

BugDiver commented Sep 6, 2019

We discussed both approaches and decided to go with the second one. Here are a few more details about it

  • During the execution whenever Gauge receives a screenshot from the runner (Failure screenshot or Captured via gauge API), It will write it file with the unique name inside the GAUGE_PROJECT_ROOT/.gauge/screenshots dir.
  • It will send the information about the screenshot (file path) in the message which gets notified to Plugins.
  • If the plugin requires screenshot it can read it from the received file path.
  • It's plugins responsibility is to convert the screenshot in the required format.

@BugDiver
Copy link
Member

BugDiver commented Sep 6, 2019

We did a spike with the discussed approach (option 2). Turns out it slows the execution time significantly. Here are some findings:

We ran a sample gauge-js project with 6 specifications. Each specification generates 6 screenshots. 5 captured via gauge API (gauge.screenshot) and 1 failure screenshot. So total number of screenshot becomes 36.

Without writing to file (The current behaviour) the execution time was 7.861 seconds.
When it writes to file the execution time becomes 11.751 seconds.

After digging more into it we found that most of this extra time is being used in encoding the base64 to png. We need to do that because Runner sends the screenshot in base64 format, and we need to convert it to PNG.
When the screenshot data is dumped to the files without conversion the execution time is 8 seconds (it's reasonable IMO).

We discussed a new approach to deal with this problem:

  • Runner write screenshots

    The runner will write the file and sends the file path to Gauge which will get passed on to plugins. Next steps will be the same as the other approached (plugin copy or read the file accordingly).

    Pros
    • Totally avoid conversion bytes -> base64 -> bytes -> PNG conversion
    • Reduce execution time
    Cons
    • Change in all the runners
    • Some configuration to set where to write screenshots.

@zabil
Copy link
Member

zabil commented Sep 6, 2019

To mitigate all above-mentioned issues we thought of streaming ExecutionResult to the plugins as soon as execution starts. It seems to be the right thing to do but then the plugins will have to own the responsibility of accumulating the result.

The proposed solution is still a workaround for the right approach. There is value in having the image data as a part of the returned message and Gauge can decide what to do with it and how to store it. Can't we just solve it the right way instead of the runner taking screenshot and only returning paths? I feel it's not the runner's responsibility to figure out saving of files. It also adds another point of failure if something goes wrong in saving the files.

@BugDiver
Copy link
Member

BugDiver commented Sep 9, 2019

Can't we just solve it the right way instead of the runner taking a screenshot and only returning paths?

Agree, If runner starts returning path, we may have to change the way custom screenshot works.
We need to discuss it more.

@BugDiver BugDiver removed their assignment Sep 9, 2019
@negiDharmendra
Copy link
Contributor Author

As @BugDiver has mentioned here Gauge writing screenshots to a file increases the execution time significantly.
To avoid degradation in execution time we will have to avoid the conversion bytes -> base64 -> bytes -> PNG. If Gauge runner starts writing the screenshot directly into a file (in a location configured by Gauge) and just share the file path in the execution result. Then the respective plugin can decide to convert it to the base64 string or can just use it as a file.

@negiDharmendra
Copy link
Contributor Author

We will have to change the contract of custom screenshot grabber to write the screenshot data to a file and return the file path.

@nehashri
Copy link
Contributor

@negiDharmendra I see one drawback with the runner writing to the file and not Gauge. The runner will be forced to run in the same machine as that of Gauge. This will limit the architecture in certain ways.

@negiDharmendra
Copy link
Contributor Author

@nehashri Agreed, this approach will be a problem if we decided to change the Gauge and runner architecture in a way where they can run on two different machines.

@negiDharmendra negiDharmendra self-assigned this Nov 18, 2019
negiDharmendra added a commit to getgauge/gauge-proto that referenced this issue Nov 18, 2019
negiDharmendra added a commit to getgauge/gauge-proto that referenced this issue Nov 18, 2019
…#22)

* Added field to share pre and post execution hook screenshot file path with the plugins. getgauge/gauge#1476

* Added deprecation warning for field to contain screenshot bytes. getgauge/gauge#1476
negiDharmendra added a commit to getgauge/html-report that referenced this issue Nov 19, 2019
negiDharmendra added a commit to getgauge/html-report that referenced this issue Nov 19, 2019
@negiDharmendra
Copy link
Contributor Author

negiDharmendra commented Nov 20, 2019

Going forward all the runner will write the screenshots into the file and just share the file path with Gauge and Gauge will do the same by sharing the screenshots file with other plugins. Having said that we have to decide on how do we handle the backward compatibility.

  • Gauge Version(With file based screenshots) , Runner(With bytes stream based screenshots), Plugins(With file based screenshots)

    In this scenario, the language runner will be sending the screenshots bytes to Gauge so the Gauge will. But the plugins will just ignore them as they will be expecting screenshots file path instead of bytes. This might create an impression on the user that the screenshot functionality is broken.

    To handle this scenario either the Gauge or the plugins will have to write screenshot to file.

  • Gauge Version(With file based screenshots) , Runner(With file based screenshots), Plugins(With bytes stream based screenshots)

    In this scenario, the language runner will be sending the screenshots file path Gauge and Gauge will do the same. But the plugins will just ignore them as they will be expecting screenshots bytes instead of the file path. This might create an impression on the user that the screenshot functionality is broken.

There are two approaches to ensure backward compatibility.

  • First approach

    For the first scenario, either the Gauge or the plugins will have to write a screenshot to file.
    For the second scenario, either the Gauge or the plugins will have to convert the screenshot to bytes from the file.

  • Second approach

    Runner and plugins can have the capability to indicate that they have support for file-based screenshots. Then gauge can decide whether to go with bytes based screenshots( when one of the plugins or runner does not have support for file-based screenshots) or file-based screenshots.

    Cons: We will have to make a release for all the plugins with the mentioned capability ev en though a plugin might not need the screenshot.

@zabil
Copy link
Member

zabil commented Nov 20, 2019

The second approach sounds confusing because it involves releasing all the plugins with the capability. This means backward compatibility is broken.

If this proving to be as complex can we just assume that version of Gauge that passes the file paths while set compatibility and force upgrade of all plugins to a version that accepts file paths?

negiDharmendra added a commit to getgauge/gauge-ruby that referenced this issue Nov 21, 2019
negiDharmendra added a commit to getgauge/gauge-ruby that referenced this issue Nov 21, 2019
negiDharmendra added a commit to getgauge/gauge-java that referenced this issue Dec 12, 2019
negiDharmendra added a commit to getgauge/gauge-java that referenced this issue Dec 16, 2019
negiDharmendra added a commit to getgauge/gauge-java that referenced this issue Dec 17, 2019
negiDharmendra added a commit to getgauge/gauge-ruby that referenced this issue Dec 17, 2019
       - Rename file_based_screengrabber to custom_screenshot_writer
       - Add missing test for configuration.
negiDharmendra added a commit to getgauge/gauge-java that referenced this issue Dec 24, 2019
negiDharmendra added a commit to getgauge/gauge-java that referenced this issue Dec 24, 2019
negiDharmendra added a commit to getgauge/gauge-java that referenced this issue Dec 24, 2019
negiDharmendra added a commit to getgauge/gauge-java that referenced this issue Dec 24, 2019
nehashri pushed a commit to getgauge/gauge-java that referenced this issue Dec 26, 2019
* Updated gauge proto message. getgauge/gauge#1476

* Use protobuf-maven-plugin to generate Java proto files.

* Refactored LspServer to Use Message builder

* Write screenshots to file and return the file path instead of returning screenshots bytes. getgauge/gauge#1476

* Introduced an interface for file based screenshot grabber. getgauge/gauge#1476

* Deprecate CustomScreenshotGrabber interface. getgauge/gauge#1476
negiDharmendra added a commit to getgauge/gauge-python that referenced this issue Jan 7, 2020
negiDharmendra added a commit to getgauge/gauge-python that referenced this issue Jan 7, 2020
negiDharmendra added a commit to getgauge/gauge-python that referenced this issue Jan 7, 2020
@Debashis9012
Copy link
Contributor

Debashis9012 commented Jan 10, 2020

Dev note:

If we define gauge_screenshots_dir environment variable in default properties then gauge should be able to create the directory and the screenshots should be saved into the same defined dir.

Current Behaviour:

  • Gauge doesn't create the dir. as per gauge_screenshots_dir environment variable in default properties

Currently to make it work as the excepted user have to create the dir. inside the project manually which should not be the case

negiDharmendra added a commit to getgauge/gauge-js that referenced this issue Jan 13, 2020
	- Change default and custome screenshot api <screenshotFn> to
          write screenshots to file.

	- Add a new API customScreenshotWriter
negiDharmendra added a commit to getgauge/gauge-js that referenced this issue Jan 13, 2020
@Debashis9012
Copy link
Contributor

This issue has been tested and found fixed.
Tested version:

Gauge version: 1.0.7
Commit Hash: 50fb6484

Plugins
-------
html-report (4.0.9)
java (0.7.4)

@cyberjaime45
Copy link

cyberjaime45 commented Feb 11, 2020

Where can I find the html-report 4.0.9 and Java .7.4?

I installed Gauge 1.0.7 with html-report 4.0.8 and Java .7.3 on a Linux machine.
Ran in parallel just 5 scenarios.

The log size is almost 1GB. I have switch back to Gauge 1.0.5.
Log size only 1mb

@tarekz
Copy link

tarekz commented Apr 22, 2020

I am not sure if this is related, but I have installed gauge 1.0.8.
I am getting the followign error at the end of the spec:
Failed to read screenhsot open **************\gauge-tests.gauge\screenshots/4hI1mNgBMgQAAAAASUVORK5CYII=:
If open the **************\gauge-tests.gauge\screenshots the screenshots are saved but using the name screenshot-343562431572700.png for example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

7 participants