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

dropping behat tests from PdoEventSourcing module #143

Merged
merged 3 commits into from
Jun 11, 2023

Conversation

unixslayer
Copy link
Member

@unixslayer unixslayer commented Jun 6, 2023

No description provided.

@unixslayer unixslayer marked this pull request as draft June 6, 2023 14:49
@unixslayer unixslayer force-pushed the main branch 3 times, most recently from 9c2d862 to 930548e Compare June 6, 2023 15:27
@unixslayer unixslayer changed the title enable pcov coverage which won't decrease pipeline performance tests improvements Jun 6, 2023
@unixslayer unixslayer force-pushed the main branch 6 times, most recently from 50eb8b5 to b5a6420 Compare June 9, 2023 13:17
@unixslayer unixslayer changed the title tests improvements dropping behat tests from PdoEventSourcing module Jun 9, 2023
@unixslayer unixslayer force-pushed the main branch 3 times, most recently from 0ee4d38 to c13f145 Compare June 9, 2023 13:35
@unixslayer unixslayer marked this pull request as ready for review June 9, 2023 13:42
dropping behat tests from PdoEventSourcing module and rewriting them using Ecotone Test Support
using pcov for coverage report
Copy link
Member

@dgafka dgafka left a comment

Choose a reason for hiding this comment

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

Amazing refactor. You've put a lot of heart into this, thanks :)


It's worth to give a context to this PR:
Ecotone Lite tests based on phpunit are much more easy to maintain than behat ones.
The behat tests were introduced before Ecotone Lite testing support were introduced and all new tests will be written in phpunit instead.

{
// fixme Calling ProjectionManager to delete the projection throws `Header with name ecotone.eventSourcing.manager.deleteEmittedEvents does not exists` exception
//$this->getGateway(ProjectionManager::class)->deleteProjection($projectionName);
$this->configuredMessagingSystem->runConsoleCommand('ecotone:es:delete-projection', ['name' => $projectionName]);
Copy link
Member

Choose a reason for hiding this comment

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

Yep, if you take a look on the defined interface it requires deleteEmittedEvents, this would be needed to add as optional to make it work

->withExtensionObjects([
EventSourcingConfiguration::createWithDefaults(),
]),
pathToRootCatalog: __DIR__ . '/../../',
Copy link
Member

Choose a reason for hiding this comment

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

Ecotone Lite have problem resolving this automatically, because for split testing it use symlinks. And it count the root catalog from symlink instead of real vendor catalog. Maybe you have idea how to solve this?

'Test\Ecotone\EventSourcing\Fixture\ProjectionFromCategoryUsingAggregatePerStream',
'Test\Ecotone\EventSourcing\Fixture\Basket',
'Test\Ecotone\EventSourcing\Fixture\Ticket',
])
Copy link
Member

Choose a reason for hiding this comment

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

I love how the namespaces help reuse scenarios

self::assertFalse($connection->createSchemaManager()->tablesExist('in_progress_tickets'));
}

public function test_building_multiple_polling_projection(): void
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adjusting this and being so precise about what we are testing. This will helpful for the maintainers in case of breaking the test

Copy link
Member Author

Choose a reason for hiding this comment

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

About that maintainers thing ... I noticed there is only one at the moment ;)

Copy link
Member

Choose a reason for hiding this comment

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

I think we can consider this one resolve now :)

Copy link
Contributor

Choose a reason for hiding this comment

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

About that maintainers thing ... I noticed there is only one at the moment ;)

Haha 😉

@dgafka dgafka merged commit 074f372 into ecotoneframework:main Jun 11, 2023
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.

3 participants