Skip to content
This repository was archived by the owner on Dec 17, 2024. It is now read-only.

Comments

#61: Event dispatcher is breaking Pretty formatter#62

Open
marcelovani wants to merge 6 commits intoelvetemedve:masterfrom
marcelovani:61-event-dispatcher-breaking-formatter
Open

#61: Event dispatcher is breaking Pretty formatter#62
marcelovani wants to merge 6 commits intoelvetemedve:masterfrom
marcelovani:61-event-dispatcher-breaking-formatter

Conversation

@marcelovani
Copy link
Contributor

@marcelovani marcelovani commented Jan 17, 2023

##What this PR does

  • Fixed the issue where Pretty formatter was not working with Cucumber json formatter
  • Removed old Event
  • Implementation using static cache for the image urls.
  • Added tag to the image uploader service, to be used by Cucumber reports to get the service using tag id.
  • Updated Cucumber PR https://github.com/cawolf/behat-cucumber-formatter/pull/12/files

@marcelovani marcelovani force-pushed the 61-event-dispatcher-breaking-formatter branch from 3bf921c to e48b7c8 Compare January 17, 2023 16:01
@marcelovani
Copy link
Contributor Author

marcelovani commented Jan 17, 2023

I debugged the listeners for service id event_dispatcher:

array(3) {
  [0]=>
  string(29) "tester.scenario_tested.before"
  [1]=>
  string(28) "tester.example_tested.before"
  [2]=>
  string(31) "tester.exercise_completed.after"
}

I am stuck. If I change the id to bex.screenshot_extension.event_dispatcher the Pretty formatter and Progress formatter will work, but the event being dispatched doesn't work, because there are no listeners.

@elvetemedve Do you have any suggestions to fix this?

@marcelovani marcelovani changed the title #61: https://github.com/elvetemedve/behat-screenshot/issues/61 #61: WIP: Event dispatcher is breaking Pretty formatter Jan 17, 2023
@elvetemedve
Copy link
Owner

Without looking into it deeper, I believe the problem is that you have two instances of the EventDispatcher one is provided by Behat itself and the 2nd is what you defined in the services.xml file of the extension.

If you give it the same service ID, it will overwrite the other, loosing the default listeners of Behat.

So I would remove the 2nd instance from service.xml and pass the original instance somehow to ScreenshotUploader (though it might be tricky to do if it's registered as a private service).

@marcelovani
Copy link
Contributor Author

I am really struggling to find a solution.

Ideally we should not have to dispatch an event, because Behat already dispatches tester.scenario_tested.before.
If we could find a way to have a soft dependency on behat-screenshot, at this point https://github.com/cawolf/behat-cucumber-formatter/pull/12/files#diff-b0c7aa6314a2629ca93e9c54ee90e3e11e3a637c35c8208f45914957e3d8ccabR260 we could try to get the behat-screenshot service and check if there is any screenshot, then we could remove a lot of code.

@elvetemedve
Copy link
Owner

How would tester.scenario_tested.before be useful here? Screenshot is uploaded later at ScenarioTested::AFTER.

Alternatively you could try to pass the screenshot file information via one of the Behat events. Though I don't see any of these classes allowing extra info added. So it needs more investigation.

@marcelovani
Copy link
Contributor Author

Your are right, I just copy and pasted without reading the event name.
I came up with a new solution, will post here soon.

@marcelovani
Copy link
Contributor Author

Description updated with new approach, please review. The nice thing is that it makes Cucumber extension configurable and no longer needs a hard dependency.

@marcelovani marcelovani changed the title #61: WIP: Event dispatcher is breaking Pretty formatter #61: Event dispatcher is breaking Pretty formatter Jan 19, 2023
* Used to statically store the list of files uploaded.
*/
private $eventDispatcher;
private static $imageUrls;
Copy link
Owner

Choose a reason for hiding this comment

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

Why is it static? This service will be kept in the service container all the time, so this instance will be available during test execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check that

With static it kind of works, but it wrong, because we can see attachments from other scenarios, so need to fix that.
Screenshot 2023-01-19 at 17 17 50

I tried without using static, when the Cucumber tries to get the values, it comes as NULL. Not sure if I am doing something wrong. Need to investigate further...
Screenshot 2023-01-19 at 17 14 50

Copy link
Owner

Choose a reason for hiding this comment

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

Me neither, but using static in an OOP language is a code smell.
Maybe you just need to subscribe to a different event and/or switch before/after.
Unfortunately I can't give you a better idea without digging down in the Behat source code.

$this->config = $config;
$this->eventDispatcher = $eventDispatcher;
$this->output = $output;
$this->config = $config;
Copy link
Owner

Choose a reason for hiding this comment

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

Indention is 4 spaces on this project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, PHP storm changes that automatically. Fixed manually.

Copy link
Owner

Choose a reason for hiding this comment

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

I should have added an EditorConfig file to the project to avoid such difficulties. :(

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants