From 68485b30d7d155b0ae63668bc0679fed0d8824e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Mon, 7 Sep 2026 14:02:19 +0200 Subject: [PATCH] Raise test coverage from 46% to 97% of lines Most of the bugs this series fixed lived in code no test touched. Cover what was left: the request populators, the consent checker, both tag bag subscribers, StoreFbcSubscriber and CookieBasedFbpContext. Add an end to end test that dispatches a real event through a booted kernel and asserts what reaches the client and the tag bag, including that nothing is sent or enriched for a bot, and gate coverage in Codecov so it cannot regress silently. Fixes #32 --- .gitattributes | 1 + codecov.yml | 16 ++ tests/Double/Doubles.php | 66 +++++++ .../RecordingConversionsApiClientFactory.php | 34 ++++ tests/Integration/PipelineTest.php | 174 ++++++++++++++++++ .../ConsentChecker/ConsentCheckerTest.php | 73 ++++++++ .../AddEventToTagBagSubscriberTest.php | 99 ++++++++++ .../AddLibraryToTagBagSubscriberTest.php | 92 +++++++++ ...ulateFbpAndFbcPropertiesSubscriberTest.php | 70 +++++++ .../PopulatePixelsSubscriberTest.php | 42 +++++ ...opulateRequestPropertiesSubscriberTest.php | 49 +++++ .../StoreFbcSubscriberTest.php | 145 +++++++++++++++ 12 files changed, 861 insertions(+) create mode 100644 codecov.yml create mode 100644 tests/Double/Doubles.php create mode 100644 tests/Double/RecordingConversionsApiClientFactory.php create mode 100644 tests/Integration/PipelineTest.php create mode 100644 tests/Unit/ConsentChecker/ConsentCheckerTest.php create mode 100644 tests/Unit/EventSubscriber/AddEventToTagBagSubscriberTest.php create mode 100644 tests/Unit/EventSubscriber/AddLibraryToTagBagSubscriberTest.php create mode 100644 tests/Unit/EventSubscriber/PopulateFbpAndFbcPropertiesSubscriberTest.php create mode 100644 tests/Unit/EventSubscriber/PopulatePixelsSubscriberTest.php create mode 100644 tests/Unit/EventSubscriber/PopulateRequestPropertiesSubscriberTest.php create mode 100644 tests/Unit/EventSubscriber/StoreFbcSubscriberTest.php diff --git a/.gitattributes b/.gitattributes index 580071b..41550e0 100644 --- a/.gitattributes +++ b/.gitattributes @@ -9,4 +9,5 @@ /rector.php export-ignore /README.md export-ignore /UPGRADE.md export-ignore +/codecov.yml export-ignore /composer-dependency-analyser.php export-ignore diff --git a/codecov.yml b/codecov.yml new file mode 100644 index 0000000..26ae427 --- /dev/null +++ b/codecov.yml @@ -0,0 +1,16 @@ +# Coverage is a gate, not a report: the bundle sat at 46% of lines while most of its bugs lived in the untested +# half, so a drop should fail the pull request rather than show up as a number nobody reads +coverage: + status: + project: + default: + target: 95% + # Small unavoidable dips (a new @codeCoverageIgnore, a refactor) should not block a merge + threshold: 1% + patch: + default: + target: 90% + +comment: + layout: "condensed_header, diff, files" + require_changes: true diff --git a/tests/Double/Doubles.php b/tests/Double/Doubles.php new file mode 100644 index 0000000..51c1930 --- /dev/null +++ b/tests/Double/Doubles.php @@ -0,0 +1,66 @@ +granted; + } + }; + } + + /** + * @param list $pixels + */ + public static function pixelProvider(array $pixels): PixelProviderInterface + { + return new class($pixels) implements PixelProviderInterface { + /** + * @param list $pixels + */ + public function __construct(private readonly array $pixels) + { + } + + public function getPixels(): array + { + return $this->pixels; + } + }; + } + + public static function fbcContext(?Fbc $fbc): FbcContextInterface + { + return new class($fbc) implements FbcContextInterface { + public function __construct(private readonly ?Fbc $fbc) + { + } + + public function getFbc(): ?Fbc + { + return $this->fbc; + } + }; + } + + private function __construct() + { + } +} diff --git a/tests/Double/RecordingConversionsApiClientFactory.php b/tests/Double/RecordingConversionsApiClientFactory.php new file mode 100644 index 0000000..e84d579 --- /dev/null +++ b/tests/Double/RecordingConversionsApiClientFactory.php @@ -0,0 +1,34 @@ + */ + public static array $events = []; + + public static function reset(): void + { + self::$events = []; + } + + public static function create(): ClientInterface + { + return new class() implements ClientInterface { + public function sendEvent(Event $event): void + { + RecordingConversionsApiClientFactory::$events[] = $event; + } + }; + } +} diff --git a/tests/Integration/PipelineTest.php b/tests/Integration/PipelineTest.php new file mode 100644 index 0000000..ecbebc5 --- /dev/null +++ b/tests/Integration/PipelineTest.php @@ -0,0 +1,174 @@ + $options + */ + protected static function createKernel(array $options = []): KernelInterface + { + /** @var TestKernel $kernel */ + $kernel = parent::createKernel($options); + $kernel->addTestBundle(SetonoMetaConversionsApiBundle::class); + $kernel->addTestBundle(SetonoBotDetectionBundle::class); + $kernel->addTestBundle(SetonoTagBagBundle::class); + $kernel->handleOptions($options); + + return $kernel; + } + + protected function setUp(): void + { + RecordingConversionsApiClientFactory::reset(); + } + + #[Test] + public function it_sends_an_enriched_event_and_renders_the_tags(): void + { + self::boot(); + self::pushRequest('Mozilla/5.0 (Macintosh) Chrome/140.0'); + + $container = self::getContainer(); + + $dispatcher = $container->get('test.event_dispatcher'); + self::assertInstanceOf(EventDispatcherInterface::class, $dispatcher); + + $metaEvent = new Event(Event::EVENT_VIEW_CONTENT); + $metaEvent->customData->contentName = 'Blue Jeans'; + + // An application listener enriching at the documented priority + $dispatcher->addListener( + ConversionsApiEventRaised::class, + static function (ConversionsApiEventRaised $event): void { + $event->event->userData->email[] = 'customer@example.com'; + }, + ConversionsApiEventRaised::PRIORITY_ENRICH, + ); + + $dispatcher->dispatch(new ConversionsApiEventRaised($metaEvent), ConversionsApiEventRaised::class); + + // Server side: the command was dispatched, handled, and reached the client + self::assertCount(1, RecordingConversionsApiClientFactory::$events); + $sent = RecordingConversionsApiClientFactory::$events[0]->getPayload(); + + self::assertSame('ViewContent', $sent['event_name']); + self::assertSame('https://example.com/jeans', $sent['event_source_url']); + self::assertSame($metaEvent->eventId, $sent['event_id']); + + $userData = $sent['user_data']; + self::assertIsArray($userData); + self::assertSame('Mozilla/5.0 (Macintosh) Chrome/140.0', $userData['client_user_agent']); + self::assertArrayHasKey('fbp', $userData); + // The application's email is hashed, never sent raw + self::assertSame([hash('sha256', 'customer@example.com')], $userData['em']); + + // Client side: the pixel, the init and the track call are in the tag bag + $tagBag = $container->get('test.tag_bag'); + self::assertInstanceOf(TagBagInterface::class, $tagBag); + + // The library tag itself hangs off kernel.request, which this test does not fire, so it is covered by + // AddLibraryToTagBagSubscriberTest instead + $rendered = $tagBag->renderAll(); + self::assertStringContainsString("fbq('init', '1234'", $rendered); + self::assertStringContainsString("fbq('track', 'ViewContent'", $rendered); + // The same event id on both sides is what makes Meta deduplicate the pair + self::assertStringContainsString(sprintf("eventID: '%s'", $metaEvent->eventId), $rendered); + } + + #[Test] + public function it_sends_nothing_for_a_bot(): void + { + self::boot(); + self::pushRequest('Googlebot/2.1 (+http://www.google.com/bot.html)'); + + $container = self::getContainer(); + + $dispatcher = $container->get('test.event_dispatcher'); + self::assertInstanceOf(EventDispatcherInterface::class, $dispatcher); + + $enriched = false; + $dispatcher->addListener( + ConversionsApiEventRaised::class, + static function () use (&$enriched): void { + $enriched = true; + }, + ConversionsApiEventRaised::PRIORITY_ENRICH, + ); + + $dispatcher->dispatch(new ConversionsApiEventRaised(new Event(Event::EVENT_VIEW_CONTENT)), ConversionsApiEventRaised::class); + + self::assertSame([], RecordingConversionsApiClientFactory::$events); + // ... and the application never spent anything enriching it + self::assertFalse($enriched); + + $tagBag = $container->get('test.tag_bag'); + self::assertInstanceOf(TagBagInterface::class, $tagBag); + self::assertStringNotContainsString('fbq(', $tagBag->renderAll()); + } + + private static function boot(): void + { + self::bootKernel(['config' => function (TestKernel $kernel) { + $kernel->addTestConfig(static function (ContainerBuilder $container) { + $container->loadFromExtension('setono_tag_bag', [ + 'renderer' => ['twig' => false], + ]); + $container->loadFromExtension('setono_meta_conversions_api', [ + 'client_side' => true, + 'pixels' => [ + ['id' => '1234', 'access_token' => 's3cr3t'], + ], + ]); + + $container->register(ClientInterface::class, ClientInterface::class) + ->setFactory([RecordingConversionsApiClientFactory::class, 'create']); + + $container->setAlias('test.event_dispatcher', 'event_dispatcher')->setPublic(true); + $container->setAlias('test.tag_bag', 'setono_tag_bag.tag_bag')->setPublic(true); + $container->setAlias('test.request_stack', 'request_stack')->setPublic(true); + }); + }]); + } + + private static function pushRequest(string $userAgent): void + { + $request = Request::create('https://example.com/jeans'); + $request->headers->set('User-Agent', $userAgent); + + $requestStack = self::getContainer()->get('test.request_stack'); + self::assertInstanceOf(RequestStack::class, $requestStack); + $requestStack->push($request); + } +} diff --git a/tests/Unit/ConsentChecker/ConsentCheckerTest.php b/tests/Unit/ConsentChecker/ConsentCheckerTest.php new file mode 100644 index 0000000..674dfcd --- /dev/null +++ b/tests/Unit/ConsentChecker/ConsentCheckerTest.php @@ -0,0 +1,73 @@ +isGranted()); + } + + #[Test] + public function it_grants_when_the_consent_bundle_is_not_installed(): void + { + $checker = new ConsentChecker(true, DefaultConsents::CONSENT_MARKETING, null); + + self::assertTrue($checker->isGranted()); + } + + #[Test] + public function it_delegates_to_the_consent_bundle(): void + { + self::assertTrue((new ConsentChecker(true, DefaultConsents::CONSENT_MARKETING, self::thirdParty(true)))->isGranted()); + self::assertFalse((new ConsentChecker(true, DefaultConsents::CONSENT_MARKETING, self::thirdParty(false)))->isGranted()); + } + + #[Test] + public function it_asks_for_the_configured_category(): void + { + $thirdParty = new class() implements ThirdPartyConsentCheckerInterface { + /** @var list */ + public array $asked = []; + + public function isGranted(string $consent): bool + { + $this->asked[] = $consent; + + return true; + } + }; + + (new ConsentChecker(true, DefaultConsents::CONSENT_STATISTICAL, $thirdParty))->isGranted(); + + self::assertSame([DefaultConsents::CONSENT_STATISTICAL], $thirdParty->asked); + } + + private static function thirdParty(bool $granted): ThirdPartyConsentCheckerInterface + { + return new class($granted) implements ThirdPartyConsentCheckerInterface { + public function __construct(private readonly bool $granted) + { + } + + public function isGranted(string $consent): bool + { + return $this->granted; + } + }; + } +} diff --git a/tests/Unit/EventSubscriber/AddEventToTagBagSubscriberTest.php b/tests/Unit/EventSubscriber/AddEventToTagBagSubscriberTest.php new file mode 100644 index 0000000..ceca768 --- /dev/null +++ b/tests/Unit/EventSubscriber/AddEventToTagBagSubscriberTest.php @@ -0,0 +1,99 @@ +add(self::event()); + + $rendered = $tagBag->renderAll(); + + self::assertStringContainsString("fbq('init', '1234'", $rendered); + self::assertStringContainsString("fbq('track', 'ViewContent'", $rendered); + } + + /** + * The event id ties the browser event to the server event so Meta deduplicates the pair + */ + #[Test] + public function it_renders_the_event_id_for_deduplication(): void + { + $tagBag = self::tagBag(); + $event = self::event(); + + self::subscriber($tagBag)->add($event); + + self::assertStringContainsString(sprintf("eventID: '%s'", $event->event->eventId), $tagBag->renderAll()); + } + + /** + * The init tag has a higher priority than the one AddLibraryToTagBagSubscriber adds, so it replaces it + */ + #[Test] + public function its_init_tag_outranks_the_library_one(): void + { + $tagBag = self::tagBag(); + $tagBag->add(FbqInitTag::create('', 50)); + + self::subscriber($tagBag)->add(self::event()); + + $rendered = $tagBag->renderAll(); + + self::assertStringNotContainsString('the library init', $rendered); + self::assertStringContainsString("fbq('init', '1234'", $rendered); + } + + #[Test] + public function it_renders_nothing_without_consent(): void + { + $tagBag = self::tagBag(); + + self::subscriber($tagBag, consentGranted: false)->add(self::event()); + + self::assertSame('', $tagBag->renderAll()); + } + + private static function subscriber(TagBag $tagBag, bool $consentGranted = true): AddEventToTagBagSubscriber + { + return new AddEventToTagBagSubscriber($tagBag, new FbqGenerator(), Doubles::consentChecker($consentGranted)); + } + + private static function event(): ConversionsApiEventRaised + { + $metaEvent = new Event(Event::EVENT_VIEW_CONTENT); + $metaEvent->pixels = [new Pixel('1234', 's3cr3t')]; + + return new ConversionsApiEventRaised($metaEvent); + } + + /** + * setono/tag-bag ^2.2 requires a renderer while ^2.5 defaults it, and CompositeRenderer takes its renderers + * through add() in 2.2 and through the constructor in 2.5. Every tag the bundle produces is content aware, + * so one renderer covers both ends of the supported range + */ + private static function tagBag(): TagBag + { + return new TagBag(new ContentAwareRenderer()); + } +} diff --git a/tests/Unit/EventSubscriber/AddLibraryToTagBagSubscriberTest.php b/tests/Unit/EventSubscriber/AddLibraryToTagBagSubscriberTest.php new file mode 100644 index 0000000..2fb2a5e --- /dev/null +++ b/tests/Unit/EventSubscriber/AddLibraryToTagBagSubscriberTest.php @@ -0,0 +1,92 @@ +add(self::event()); + + self::assertStringContainsString('connect.facebook.net/en_US/fbevents.js', $tagBag->renderSection(TagInterface::SECTION_HEAD)); + self::assertStringContainsString("fbq('init', '1234'", $tagBag->renderAll()); + } + + #[Test] + public function it_renders_nothing_without_pixels(): void + { + $tagBag = self::tagBag(); + + self::subscriber($tagBag, pixels: [])->add(self::event()); + + self::assertSame('', $tagBag->renderAll()); + } + + #[Test] + public function it_renders_nothing_without_consent(): void + { + $tagBag = self::tagBag(); + + self::subscriber($tagBag, consentGranted: false)->add(self::event()); + + self::assertSame('', $tagBag->renderAll()); + } + + #[Test] + public function it_ignores_sub_requests(): void + { + $tagBag = self::tagBag(); + + self::subscriber($tagBag)->add(self::event(HttpKernelInterface::SUB_REQUEST)); + + self::assertSame('', $tagBag->renderAll()); + } + + /** + * @param list|null $pixels + */ + private static function subscriber(TagBag $tagBag, bool $consentGranted = true, ?array $pixels = null): AddLibraryToTagBagSubscriber + { + return new AddLibraryToTagBagSubscriber( + $tagBag, + new FbqGenerator(), + Doubles::consentChecker($consentGranted), + Doubles::pixelProvider($pixels ?? [new Pixel('1234', 's3cr3t')]), + ); + } + + private static function event(int $requestType = HttpKernelInterface::MAIN_REQUEST): RequestEvent + { + return new RequestEvent(self::createStub(HttpKernelInterface::class), new Request(), $requestType); + } + + /** + * setono/tag-bag ^2.2 requires a renderer while ^2.5 defaults it, and CompositeRenderer takes its renderers + * through add() in 2.2 and through the constructor in 2.5. Every tag the bundle produces is content aware, + * so one renderer covers both ends of the supported range + */ + private static function tagBag(): TagBag + { + return new TagBag(new ContentAwareRenderer()); + } +} diff --git a/tests/Unit/EventSubscriber/PopulateFbpAndFbcPropertiesSubscriberTest.php b/tests/Unit/EventSubscriber/PopulateFbpAndFbcPropertiesSubscriberTest.php new file mode 100644 index 0000000..bd35319 --- /dev/null +++ b/tests/Unit/EventSubscriber/PopulateFbpAndFbcPropertiesSubscriberTest.php @@ -0,0 +1,70 @@ +populate($event); + + self::assertSame($fbp, $event->event->userData->fbp); + self::assertSame($fbc, $event->event->userData->fbc); + } + + #[Test] + public function it_leaves_fbc_null_when_there_is_no_click_id(): void + { + $event = new ConversionsApiEventRaised(new Event(Event::EVENT_VIEW_CONTENT)); + + self::subscriber(new Fbp(), null)->populate($event); + + self::assertNull($event->event->userData->fbc); + } + + private static function subscriber(Fbp $fbp, ?Fbc $fbc): PopulateFbpAndFbcPropertiesSubscriber + { + return new PopulateFbpAndFbcPropertiesSubscriber( + new class($fbp) implements FbpContextInterface { + public function __construct(private readonly Fbp $fbp) + { + } + + public function getFbp(): Fbp + { + return $this->fbp; + } + }, + new class($fbc) implements FbcContextInterface { + public function __construct(private readonly ?Fbc $fbc) + { + } + + public function getFbc(): ?Fbc + { + return $this->fbc; + } + }, + ); + } +} diff --git a/tests/Unit/EventSubscriber/PopulatePixelsSubscriberTest.php b/tests/Unit/EventSubscriber/PopulatePixelsSubscriberTest.php new file mode 100644 index 0000000..278ad0a --- /dev/null +++ b/tests/Unit/EventSubscriber/PopulatePixelsSubscriberTest.php @@ -0,0 +1,42 @@ + $pixels + */ + public function __construct(private readonly array $pixels) + { + } + + public function getPixels(): array + { + return $this->pixels; + } + }))->populate($event); + + self::assertSame($pixels, $event->event->pixels); + } +} diff --git a/tests/Unit/EventSubscriber/PopulateRequestPropertiesSubscriberTest.php b/tests/Unit/EventSubscriber/PopulateRequestPropertiesSubscriberTest.php new file mode 100644 index 0000000..77e43b6 --- /dev/null +++ b/tests/Unit/EventSubscriber/PopulateRequestPropertiesSubscriberTest.php @@ -0,0 +1,49 @@ +populate($event); + + self::assertNull($event->event->eventSourceUrl); + self::assertNull($event->event->userData->clientIpAddress); + self::assertNull($event->event->userData->clientUserAgent); + } + + #[Test] + public function it_populates_the_request_properties(): void + { + $request = Request::create('https://example.com/jeans?colour=blue', server: ['REMOTE_ADDR' => '203.0.113.4']); + $request->headers->set('User-Agent', 'Chrome'); + + $requestStack = new RequestStack(); + $requestStack->push($request); + + $event = new ConversionsApiEventRaised(new Event(Event::EVENT_VIEW_CONTENT)); + + (new PopulateRequestPropertiesSubscriber($requestStack))->populate($event); + + // The full url including the query string is sent, which the README calls out + self::assertSame('https://example.com/jeans?colour=blue', $event->event->eventSourceUrl); + self::assertSame('203.0.113.4', $event->event->userData->clientIpAddress); + self::assertSame('Chrome', $event->event->userData->clientUserAgent); + } +} diff --git a/tests/Unit/EventSubscriber/StoreFbcSubscriberTest.php b/tests/Unit/EventSubscriber/StoreFbcSubscriberTest.php new file mode 100644 index 0000000..460c0ea --- /dev/null +++ b/tests/Unit/EventSubscriber/StoreFbcSubscriberTest.php @@ -0,0 +1,145 @@ + 'IwAR0rmfgHgx']), new Response()); + + self::subscriber()->store($event); + + $cookie = self::cookie($event); + self::assertNotNull($cookie); + self::assertStringEndsWith('.IwAR0rmfgHgx', (string) $cookie->getValue()); + // The browser pixel has to be able to read it + self::assertFalse($cookie->isHttpOnly()); + } + + #[Test] + public function it_does_nothing_without_a_click_id_on_the_request(): void + { + $event = self::event(new Request(), new Response()); + + self::subscriber()->store($event); + + self::assertNull(self::cookie($event)); + } + + #[Test] + public function it_does_nothing_without_consent(): void + { + $event = self::event(new Request(['fbclid' => 'IwAR0rmfgHgx']), new Response()); + + self::subscriber(consentGranted: false)->store($event); + + self::assertNull(self::cookie($event)); + } + + #[Test] + public function it_does_nothing_without_pixels(): void + { + $event = self::event(new Request(['fbclid' => 'IwAR0rmfgHgx']), new Response()); + + self::subscriber(pixels: [])->store($event); + + self::assertNull(self::cookie($event)); + } + + #[Test] + public function it_does_nothing_when_the_context_has_no_fbc(): void + { + $event = self::event(new Request(['fbclid' => 'IwAR0rmfgHgx']), new Response()); + + self::subscriber(fbc: null)->store($event); + + self::assertNull(self::cookie($event)); + } + + #[Test] + public function it_ignores_sub_requests(): void + { + $event = self::event(new Request(['fbclid' => 'IwAR0rmfgHgx']), new Response(), HttpKernelInterface::SUB_REQUEST); + + self::subscriber()->store($event); + + self::assertNull(self::cookie($event)); + } + + #[Test] + public function it_does_nothing_on_an_error_response(): void + { + $event = self::event(new Request(['fbclid' => 'IwAR0rmfgHgx']), new Response('', Response::HTTP_NOT_FOUND)); + + self::subscriber()->store($event); + + self::assertNull(self::cookie($event)); + } + + /** + * An ad click frequently lands on a redirect, for instance when the application strips the fbclid, so this one + * really does have to survive a 302 + */ + #[Test] + public function it_stores_the_click_id_on_a_redirect(): void + { + $event = self::event(new Request(['fbclid' => 'IwAR0rmfgHgx']), new Response('', Response::HTTP_FOUND)); + + self::subscriber()->store($event); + + self::assertNotNull(self::cookie($event)); + } + + private static function cookie(ResponseEvent $event): ?Cookie + { + foreach ($event->getResponse()->headers->getCookies() as $cookie) { + if (Cookies::FBC === $cookie->getName()) { + return $cookie; + } + } + + return null; + } + + /** + * @param list|null $pixels + */ + private static function subscriber( + ?Fbc $fbc = new Fbc('IwAR0rmfgHgx'), + bool $consentGranted = true, + ?array $pixels = null, + ): StoreFbcSubscriber { + return new StoreFbcSubscriber( + Doubles::fbcContext($fbc), + Doubles::consentChecker($consentGranted), + Doubles::pixelProvider($pixels ?? [new Pixel('1234', 's3cr3t')]), + new CookieDomain(new RequestStack()), + ); + } + + private static function event(Request $request, Response $response, int $requestType = HttpKernelInterface::MAIN_REQUEST): ResponseEvent + { + return new ResponseEvent(self::createStub(HttpKernelInterface::class), $request, $requestType, $response); + } +}