Skip to content

chore: add tests for JWK ASN1 fix#631

Open
bshaffer wants to merge 1 commit intomainfrom
add-tests-for-jwk-parsing
Open

chore: add tests for JWK ASN1 fix#631
bshaffer wants to merge 1 commit intomainfrom
add-tests-for-jwk-parsing

Conversation

@bshaffer
Copy link
Copy Markdown
Collaborator

Adds a test for the fix in #628

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the JWKTest suite by introducing a data provider for testing multiple JWK sets and verifying public keys against expected PEM files. It also refactors the test class to use setUpBeforeClass for initialization. I have reviewed the changes and suggest using trim() when comparing PEM strings to ensure the tests are robust against potential differences in line endings or trailing whitespace.

Comment on lines +109 to +112
$this->assertEquals(
file_get_contents(__DIR__ . '/data/' . $pubkeyFile),
$publicKey
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Comparing PEM strings directly can be brittle due to differences in line endings (CRLF vs LF) or trailing newlines across different operating systems and OpenSSL versions. It is recommended to trim() both the expected and actual strings to make the test more robust and avoid failures caused by trailing whitespace.

        $this->assertEquals(
            trim(file_get_contents(__DIR__ . '/data/' . $pubkeyFile)),
            trim($publicKey)
        );

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.

2 participants