Feature/formatter unit test #479

andrestejerina97 · 2026-01-05T19:17:16Z

ref: https://app.clickup.com/t/86b80t46g

Copilot

Pull request overview

This pull request adds comprehensive unit tests for audit log formatters in the OpenTelemetry module. The tests validate that all formatter classes can be instantiated, handle various event types correctly, and gracefully manage error conditions.

Key changes:

Added a FormatterTestHelper class with assertion methods to validate formatter implementation requirements
Added an AuditContextBuilder class using the builder pattern to simplify test context creation
Added comprehensive integration tests in AllFormattersIntegrationTest that discover and validate all formatters in the codebase
Modified AuditLogFormatterFactory to add exception handling when loading configuration

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File	Description
tests/OpenTelemetry/Formatters/Support/FormatterTestHelper.php	Helper class providing assertion methods for validating formatter implementations
tests/OpenTelemetry/Formatters/Support/AuditContextBuilder.php	Builder class for creating test audit contexts with fluent interface
tests/OpenTelemetry/Formatters/AllFormattersIntegrationTest.php	Comprehensive integration tests that validate all formatter classes
app/Audit/AuditLogFormatterFactory.php	Added exception handling around config loading in constructor

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-08T20:37:05Z

tests/OpenTelemetry/Formatters/AllFormattersIntegrationTest.php

+        if ($formatter !== null) {
+            $this->assertInstanceOf(
+                IAuditLogFormatter::class,
+                $formatter,
+                'Factory must return IAuditLogFormatter instance for valid subject'
+            );
+
+            $this->assertNotNull($formatter, 'Returned formatter must not be null');
+        }


This assertion is redundant and unreachable. If formatter is null, the condition on line 348 would be true and the code would not reach this line. If formatter is not null, then the check on line 348 already ensured it's an instance of IAuditLogFormatter, making this assertion redundant.

Suggested change

if ($formatter !== null) {

$this->assertInstanceOf(

IAuditLogFormatter::class,

$formatter,

'Factory must return IAuditLogFormatter instance for valid subject'

);

$this->assertNotNull($formatter, 'Returned formatter must not be null');

}

$this->assertNotNull($formatter, 'Returned formatter must not be null');

$this->assertInstanceOf(

IAuditLogFormatter::class,

$formatter,

'Factory must return IAuditLogFormatter instance for valid subject'

);

Copilot · 2026-01-08T20:37:07Z

tests/OpenTelemetry/Formatters/Support/FormatterTestHelper.php

+                throw new \Exception("Formatter class does not exist: {$formatterClass}");
+            }
+
+            $reflection = new ReflectionClass($formatterClass);
+
+            try {
+                $formatter = $reflection->newInstance($eventType);
+            } catch (\Throwable $e) {
+                $formatter = $reflection->newInstance();
+            }
+
+            if (!$formatter instanceof IAuditLogFormatter) {
+                throw new \Exception("Formatter must implement IAuditLogFormatter");
+            }
+
+            return $formatter;
+        } catch (\ReflectionException $e) {
+            throw new \Exception("Failed to instantiate {$formatterClass}: " . $e->getMessage());


The generic Exception class is being thrown instead of a more specific exception type. Consider using a specific exception type like InvalidArgumentException or RuntimeException to better indicate the nature of the error.

Copilot · 2026-01-08T20:37:08Z

tests/OpenTelemetry/Formatters/Support/FormatterTestHelper.php

+            throw new \Exception(
+                get_class($formatter) . " must have a setContext method"
+            );
+        }


The generic Exception class is being thrown instead of a more specific exception type. Consider using a specific exception type like InvalidArgumentException or RuntimeException to better indicate the nature of the error.

Copilot · 2026-01-08T20:37:08Z

tests/OpenTelemetry/Formatters/Support/FormatterTestHelper.php

+            throw new \Exception(
+                get_class($formatter) . " must handle empty changesets gracefully: " . $e->getMessage()
+            );


The generic Exception class is being thrown instead of a more specific exception type. Consider using a specific exception type like InvalidArgumentException or RuntimeException to better indicate the nature of the error.

Copilot · 2026-01-08T20:37:08Z

app/Audit/AuditLogFormatterFactory.php

-        $this->config = config('audit_log', []);
+        try {
+            $this->config = config('audit_log', []);
+        } catch (\Exception $e) {


This catch block silently suppresses all exceptions when attempting to load the audit_log config. If the config system throws an exception, it could indicate a more serious configuration issue that should be logged or handled explicitly rather than silently defaulting to an empty array.

Suggested change

} catch (\Exception $e) {

} catch (\Exception $e) {

Log::error('Failed to load audit_log configuration', ['exception' => $e]);

Copilot · 2026-01-08T20:37:09Z

tests/OpenTelemetry/Formatters/Support/FormatterTestHelper.php

+                throw new \Exception("Cannot test abstract formatter: {$formatterClass}");
+            }
+
+            $constructor = $reflection->getConstructor();
+            if ($constructor === null) {
+                return;
+            }
+
+            try {
+                $reflection->newInstance(IAuditStrategy::EVENT_ENTITY_CREATION);
+                return;
+            } catch (\Throwable $e) {
+                try {
+                    $reflection->newInstance();
+                    return;
+                } catch (\Throwable $e) {
+                    $requiredParams = [];
+                    foreach ($constructor->getParameters() as $param) {
+                        if (!$param->isOptional() && !$param->allowsNull()) {
+                            $requiredParams[] = $param->getName();
+                        }
+                    }
+
+                    if (!empty($requiredParams)) {
+                        throw new \Exception(
+                            "{$formatterClass} has required constructor parameters: " . 
+                            implode(', ', $requiredParams) . 
+                            ". These parameters must either have default values or be optionally injectable."
+                        );
+                    }
+                    throw $e;
+                }
+            }
+        } catch (\ReflectionException $e) {
+            throw new \Exception("Failed to validate constructor for {$formatterClass}: " . $e->getMessage());
+        }


The generic Exception class is being thrown instead of a more specific exception type. Consider using a specific exception type like InvalidArgumentException or RuntimeException to better indicate the nature of the error.

Copilot · 2026-01-08T20:37:09Z

tests/OpenTelemetry/Formatters/Support/FormatterTestHelper.php

+            throw new \Exception(
+                get_class($formatter) . " must handle invalid subjects gracefully: " . $e->getMessage()
+            );


The generic Exception class is being thrown instead of a more specific exception type. Consider using a specific exception type like InvalidArgumentException or RuntimeException to better indicate the nature of the error.

Copilot · 2026-01-08T20:37:09Z

tests/OpenTelemetry/Formatters/Support/FormatterTestHelper.php

+                throw new \Exception(
+                    "{$formatterClass} must extend AbstractAuditLogFormatter"
+                );
+            }
+        } catch (\ReflectionException $e) {
+            throw new \Exception("Failed to validate {$formatterClass}: " . $e->getMessage());
+        }


The generic Exception class is being thrown instead of a more specific exception type. Consider using a specific exception type like InvalidArgumentException or RuntimeException to better indicate the nature of the error.

Copilot · 2026-01-08T20:37:09Z

tests/OpenTelemetry/Formatters/Support/FormatterTestHelper.php

+                throw new \Exception(
+                    "{$formatterClass} must have a format() method"
+                );
+            }
+
+            $method = $reflection->getMethod('format');
+
+            if ($method->isAbstract()) {
+                throw new \Exception(
+                    "{$formatterClass}::format() must not be abstract"
+                );
+            }
+
+            $params = $method->getParameters();
+            if (count($params) < 1) {
+                throw new \Exception(
+                    "{$formatterClass}::format() must accept at least 1 parameter (subject)"
+                );
+            }
+        } catch (\ReflectionException $e) {
+            throw new \Exception("Failed to validate format method for {$formatterClass}: " . $e->getMessage());
+        }


The generic Exception class is being thrown instead of a more specific exception type. Consider using a specific exception type like InvalidArgumentException or RuntimeException to better indicate the nature of the error.

smarcet

LGTM

andrestejerina97 force-pushed the feature/formatter-unit-test branch from 590d5f6 to a044a1b Compare January 6, 2026 18:20

andrestejerina97 closed this Jan 6, 2026

andrestejerina97 force-pushed the feature/formatter-unit-test branch from a044a1b to ca920e2 Compare January 6, 2026 18:55

andrestejerina97 reopened this Jan 6, 2026

andrestejerina97 force-pushed the feature/formatter-unit-test branch from 9054f4b to bf23fcd Compare January 6, 2026 18:57

feat: add unit test for formatters

25bc9cc

andrestejerina97 force-pushed the feature/formatter-unit-test branch from bf23fcd to 25bc9cc Compare January 6, 2026 20:19

andrestejerina97 marked this pull request as ready for review January 6, 2026 20:20

andrestejerina97 requested a review from smarcet January 6, 2026 20:20

smarcet requested a review from Copilot January 8, 2026 20:33

Copilot started reviewing on behalf of smarcet January 8, 2026 20:33 View session

Copilot AI reviewed Jan 8, 2026

View reviewed changes

smarcet approved these changes Jan 8, 2026

View reviewed changes

smarcet merged commit 1147975 into main Jan 8, 2026
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/formatter unit test #479

Feature/formatter unit test #479

andrestejerina97 commented Jan 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

Copilot AI Jan 8, 2026

Uh oh!

smarcet left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	} catch (\Exception $e) {
	} catch (\Exception $e) {
	Log::error('Failed to load audit_log configuration', ['exception' => $e]);

Feature/formatter unit test #479

Feature/formatter unit test #479

Conversation

andrestejerina97 commented Jan 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

smarcet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants