Skip to content

Take screenshots and insert them in the json file#12

Open
marcelovani wants to merge 12 commits intocawolf:masterfrom
marcelovani:10-screenshots-support
Open

Take screenshots and insert them in the json file#12
marcelovani wants to merge 12 commits intocawolf:masterfrom
marcelovani:10-screenshots-support

Conversation

@marcelovani
Copy link

@marcelovani marcelovani commented Nov 30, 2022

Issue #10

What it does

  • Introduces a new configuration to specify the Behat extension that provides screenshots
  • The container will first find services with tag id screenshot.service and compare the class vs configuration
  • Updated readme
  • Keeps track of files for each step
  • Sends the files to json output

Testing
Add these entries on composer.json

  "repositories": [
    {
      "type": "vcs",
      "url": "https://github.com/marcelovani/behat-screenshot.git"
    },
    {
      "type": "vcs",
      "url": "https://github.com/marcelovani/behat-cucumber-formatter.git"
    }
  ],

then

  "require-dev": {
    "cawolf/behat-cucumber-json-formatter": "dev-10-screenshots-support",
    "bex/behat-screenshot": "dev-61-event-dispatcher-breaking-formatter"
  }

Screenshot
Screenshot 2022-11-30 at 00 46 02

@marcelovani marcelovani changed the title #10 Added event listener to keep track up uploaded files and push to … Take screenshots and insert them in the json file Dec 1, 2022
@marcelovani
Copy link
Author

New approach, description updated

Copy link
Owner

@cawolf cawolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! I left some small notes for you to review.

README.md Outdated
- `fileName` _(optional)_: Filename of generated report - current feature name will be used by default.
Only applicable when `resultFilePerSuite` is not enabled.
- `resultFilePerSuite` _(optional)_: The default behaviour is to generate a single report named `all.json`.
- `screenshotExtension` _(optional)_: The name of the extension to be used to take screenshots.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should be a line lower. The next line ("If this option is set to true..." belongs to the resultFilePerSuite option, not to the screenshotExtension.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

// Integration with other Behat extensions that provide screenshot services.
$imageUploaderService = null;
if (!empty($config['screenshotExtension'])) {
$tags = $container->findTaggedServiceIds('screenshot.service');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not check the referenced package too deeply, but I imagine that this service tag is introduced by the other package?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$service = $container->get($id);
// Check if the configuration for the screenshot extension matches the namespace of the service.
if (strpos(get_class($service), $config['screenshotExtension']) !== false) {
$imageUploaderService = $service;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code leads to the last of all matching services being selected as imageUploaderService, not the first. I assume this is intended behaviour?
If so, a short comment would be nice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was not intentional. I added a break.
If you think we should change this behaviour, please let me know.

@marcelovani marcelovani requested a review from cawolf March 8, 2023 08:28
@marcelovani marcelovani force-pushed the 10-screenshots-support branch from 1ee56dd to 6a20e6c Compare May 20, 2023 08:44
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.

2 participants