Looking good so far @geckod22
Let's add a PHPUnit test to validate that the event is removed when the attempt limit is hit. We should also have a unit test to validate that the timestamp sent to Hiive matches the one in the database.

Looking good so far @geckod22 Let's add a PHPUnit test to validate that the event is removed when the attempt limit is hit. We should also have a unit test to validate that the timestamp sent to Hiive matches the one in the database.

Hi @wpscholar about the timestamp validation, it seems the events are taken from the database where the created_at is not changed (we only increment the attempts column), so on retry they still have the same timestamp; or do you mean a different thing?

Looking good so far @geckod22 Let's add a PHPUnit test to validate that the event is removed when the attempt limit is hit. We should also have a unit test to validate that the timestamp sent to Hiive matches the one in the database.

Hi @wpscholar about the timestamp validation, it seems the events are taken from the database where the created_at is not changed (we only increment the attempts column), so on retry they still have the same timestamp; or do you mean a different thing?

@geckod22 Yes, we want to always pass the created_at value from the database. This makes it easy to de-dupe in the event we successfully send more than one event.

Hi @wpscholar there's a point here which is not pretty clear, so I wonder what I don't understand.
We store in database all events whose status is "failed".
Any time we create a queue of events, we retrieve existing failed events from the database and for each of them we attempt again. In case the new attempt produces a success, we forward the event to Hiive.
There's no action in the middle of this process, so the timestamp we sent to Hiive is the one we retrieved from the database, there's no possibility is different, so my concern is what we should test here with a PHP unit?
Thanks for your time :)

Hi @wpscholar there's a point here which is not pretty clear, so I wonder what I don't understand. We store in database all events whose status is "failed". Any time we create a queue of events, we retrieve existing failed events from the database and for each of them we attempt again. In case the new attempt produces a success, we forward the event to Hiive. There's no action in the middle of this process, so the timestamp we sent to Hiive is the one we retrieved from the database, there's no possibility is different, so my concern is what we should test here with a PHP unit? Thanks for your time :)

@AleTorrisi Previously, we didn't send the timestamp from the database. This has since been corrected, but I mentioned that we should have a test for it, as I don't think we ever created a test for this. I want to make sure we never have a regression. Last time we had attempted retries, having a unique timestamp every time we sent an event presented significant issues with de-duping and getting correct data. Before we try this again, I want to ensure we have a test that validates the timestamp isn't changing with each retry.

@wpscholar That’s ok, we didn’t know that behavior was different before and that created_at was not set on event.
That said, me and Armando see reasonable apply a change in the module and use the event creation date as value to store in the database.
At the moment, when an event is pushed in the database, we populate the created_at column with the current timestamp

$time = current_time( 'mysql' );
Path: wp-module-data/includes/EventQueue/Queues/BatchQueue.php on line 38

This way will not be possible have a discrepancy between the event and database data.

Do you agree with this solution?

The issue with creation date has been fixed in this
#137

project coverage report 39% @ c0e7d5e

@wpscholar last commit should suits well your request

@wpscholar default value set at 1

@arunshenoy99, @BrianHenryIE wanted to add some unit tests here before merging.

Aiming at getting them added today and we can still merge and tag a module release.