Serenity/JS should offer a more intelligent strategy to take photos of failures

[viper3400]

What's the problem you're trying to solve?

We have the PhotoTakingStrategycalled TakePhotosOfFailures. But most times it helps to see the screenshots fron before that leaded to this failure, as well. We can use the strategy TakePhotosOfInteractions to generate this helpful screenshots, but as a pitfall we generate a lot of screenshots and a lot of data. Especially in combination with nested Tasks this strategy can get very verbose.

How would you like to solve it?

We take photos of interactions for the whole scene, but if it finishes with SUCCESS, we remove the photos from the filesystem, or at least prevent them from getting part of reporting.

Are there any alternatives?

We reduce the amount of photos taken by a new TakePhotosOfTasks strategy, that will check, if an interaction is part of a Task or a nested Task, and if so, no photo is taken. Drawback: Depending on how you tasks are tailored, this can lead to missing screenshots of important interactions.

How can we make it happen?

• work on this myself (with Serenity/JS team guidance)
• hire the Serenity/JS team to build it for me
• add it to the Serenity/JS backlog and wait for a volunteer

[jan-molak]

It's a good idea. I can imagine something like TakePhotosOfInteractionsLeadingToFailure or something similar (and maybe less of a mouthful ;) ).

An interesting challenge we're going to have is where to accumulate the photos that may or may not need to be discarded.

For example, the new implementation of the PhotoTakingStategy could take screenshots upon any interaction, but not emit the ActivityRelatedArtifactGenerated events and instead store them in memory.

So assuming there's some internal events field, we could have something like this:

-            stage.announce(new ActivityRelatedArtifactGenerated(
+            this.events.push(new ActivityRelatedArtifactGenerated(
                event.sceneId,
                event.activityId,
                photoName,
                Photo.fromBase64(screenshot),
                stage.currentTime(),
            ));

Then, upon a failure, all artifacts cached internally could be emitted together. Since they would be already associated with a scene, activity, and the time the photo was taken, SerenityBDDReporter could then attach them to the appropriate steps in the report. If the scenario finished with success, the strategy could clear any cached photos.

Emitting all ActivityRelatedArtifactGenerated together should work correctly, I think, provided that they're preceded by AsyncOperationAttempted and succeeded by AsyncOperationCompleted. This will make Serenity/JS wait until the screenshots are processed and attached to the report.

Serenity BDD reporter already temporarily caches screenshots in memory (using DomainEventQueue), so referencing the same event objects in the new PhotoTakingStrategy should have a negligible impact on memory consumption.

---

Hmm, I actually quite like this behaviour. I wonder if this is how TakePhotosOfFailures should behave by default.

[viper3400]

Thought a bit about this. Actually, we're not changing the PhotoTakingStrategybut introducing a PhotoSavingStrategy.

Today, PhotoSavingStrategy initiates the saving of the artifact directly by the stage.annonunce(). I already gave the Photographerthe control to announce the ActivityRelatedArtifactGeneratedevent. Functionality is so far not affected by this change. (Not sure if it must be surrounded with the handling of an AsyncOperation, though?)

As you already said: We now have to queue this somehow until we now more about the outcome of the test. But this will completely change the behaviour and will cause a lot of tests to fail, because Photographeruntil now was not aware of outer events than some ActivityStarts and ActivityFinished.

I wonder if this is how TakePhotosOfFailures should behave by default.

Making this the default would also imply the PhotoTakingStrategyis ,at least, TakePhotosOfInteractions. And this has an impact on test execution performance, as taking the screenshot by the browser automation tool consumes some extra time. So not everyone may be interested in this behaviour.

So maybe we should go into something like a PhotoSavingStrategy: SaveAlways,SaveOnFailure?

[viper3400]

Added a second commit, but this does not work as expected. I think, I do not understand the order of the DomainEvents.

[viper3400]

Well, I found the array holding my artefacts was not initialized and so undefined. The I had to announce the ActivityRelatedArtifactGenerated in PhotoTakingStrategy.

Result so far: A screenshot is not saved immediately after it was taken, but collected into an array. Screenshots are now saved when TestRunFinishes. SerenityBDDReporter still receives the right filename and therefore the screenshots are displayed in the report.

But this help to solve the problem of this feature request? 😄

No, because the way it's build now we still have to announce before TestRunFinishes to have it in the report. And this is before we now, if the test run failed.

As I already wrote in our elements chat, I think, there is a bit missing in the eventing concept. Maybe some event like TestRunFailed, TestRunSucceeded, TestRunPending before (or instead) of TestRunFinished?

[viper3400]

Simplified sequence diagram of current situation:

sequenceDiagram
autonumber

participant TestRunner
participant EventBus
participant Photographer
participant BDDReporter
participant ArtifactArchiver
participant FileSystem

TestRunner ->> EventBus: TaskStarted
activate TestRunner
loop
TestRunner ->> EventBus: ActivityStarted / Finished
EventBus -->> Photographer: ActivityStarted / Finished
activate Photographer
Photographer ->> Photographer: takePhoto
Photographer ->> EventBus: ActivityRelatedArtifactGenerated
deactivate Photographer

par
EventBus -->> BDDReporter: notifyOf(ArtifactGenerated)
activate BDDReporter
BDDReporter ->> BDDReporter: enque DomainEvent (with filename)
deactivate BDDReporter
and
EventBus -->> ArtifactArchiver: notifyOf(ArtifactGenerated)
activate ArtifactArchiver
ArtifactArchiver ->> FileSystem: write to
ArtifactArchiver ->> EventBus: ArtifactArchived
deactivate ArtifactArchiver
end
end
TestRunner ->> EventBus: TaskFinishes
EventBus -->> BDDReporter: TaskFinishes
activate BDDReporter
BDDReporter ->> BDDReporter: processQueue()
BDDReporter ->> EventBus: ArtifactGenerated()
deactivate BDDReporter
EventBus -->> ArtifactArchiver: ArtifactGenerated
activate ArtifactArchiver
ArtifactArchiver ->> FileSystem: write to
ArtifactArchiver ->> EventBus: ArtifactArchived
deactivate ArtifactArchiver
TestRunner ->> EventBus: TaskFinished
deactivate TestRunner

[viper3400]

This is just a reminder for me from my debugging session:

In the SerenityBDDReport event queue we have both, the ActivityRelatedArtifactGenerated event that contains the base64 encoded image and the ActivityRelatedArtifactArchived event that contains the name of the png in the filesystem.

The SceneFinished event contains the Outcome of the test. So according to the different outcomes, we can process images differently.

[viper3400]

@jan-molak, I overthought this a bit by all my experiments and all your input by now:

First, taking screenshots and using Photographer makes just sense in context of browser automation. It wraps the ability of different browser automation tools to take screenshots while automating the browser.

The Photographercan use a PhotoTakingStrategy. This PhotoTakingStrategywill affect the timing when executing the test - because making screenshots here and there are additional steps with additional execution time. Then it's basically not about a strategy. Basically not about if a photo of a certain interaction was taken under certain circumstances.

It's about if a certain collection of photos should be included in the report under certain circumstances.

So the behaviour proposed in this request should not be default behaviour of TakePhotosOfFailures, as it will change execution timing in a way that may not be intended.

About the eventing:

When a photo was taken it's announced to the stage (AcitivityRelatedArtifactGenerated with artifact of type Photo).

The ArtifactArchivergets notified of the new screenshot (and will save the png file to disk if it's type of Photo. ArtifactArchiverthen announces an ActivityRelatedArtifactArchivedto stage.

The SerenityBDDReportergets notified of both events, first of the ActivityRelatedArtifactGenerated and next of the ActivityRelatedArtficatArchived. Then it pushs both events into it's DomainEvenQueue.

While the ActivityRelatedArtifactGenerated event contains the 64base encoded string of the screenshot the ActivityRelatedArtficatArchived contains just the file path to the png.

When the test runner announces the TestRunFinishes to the Stagethe SerenityBDDReportergets notified as well. It then starts to process the content of the DomainEventQueue. Result of this is a json report object (SerinityBDDReport).

ActivityRelatedArtifactGenerated event is ignored in this processing, if the type of artifact is Photo.

When processing has finished, Stagegets announced of ArtifactGenerated to notify the ArtifactArchiver again to write the json object to disk.

Question is, if the SerenityBDDReportis the only consumer of the screenshots, or are there any other consumers? If not, than we can make SerenityBDDReporterresponsible for saving the screenshots, depending on the Outcomeof a test run.

Though, there is another problem: ArtifactArchiverarchives artifacts asynchronous. I guess, it's just by happy timing that in the current implementation the last images are already saved to disk and the last onActivityRelatedArtifactArchivedevents were received by the SerenityBBDReporterbefore TestRunFinishes.

Or is there a mechanism in place, that prevents a test run to finish as long as not all async operations returned with AsyncOperationCompletedevent?

So either we let the SerenityBDDReporterwrite the photos synchronously to disk and then push the events with the filenames into the DomainEventQueue before processing the queue to produce the json.

Or, alternative we would delete the pngs from disk and the related events from the queue before processing.

This would maybe be the easier way, and maybe the more consistent one, at all?

SerenityBDDReporter.whoWill(keepPhotosOfAllTestRuns)
`SerentiyBDDReporter.whoWill(keepPhotosOfFailingTestRuns)

[viper3400]

Hi @jan-molak, I'm back on this one and overthought it a bit. You mentioned in one of our discussions that you think about making the SerenityBddReporteritself responsible for archiving the photo instead of delegating it to the ArtifactArchiver. I'm not sure if this is the best way.

• I really like the separated responsibilities.
• I don't look at the SerenityBddReporteras the only consumer of the Photographers photos.
  • Imagine for example some reporter, that will create you some markup reports including screenshots.

For solving the problem in this issue:

• having a ArtifactDeletionRequestedevent
• handled by a delete method in the ArtifactArchiver
• SerenityBddReportget a new parameter keepJustPhotosOfFailures
• If set: In the report processors do not push the screenshots into the reports
• If set: Raise the ArtifactDeletionRequestedevent instead

What do you think?