Skip to content

Conversation

@ActuallyConnor
Copy link
Contributor

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

N/A

Describe the solution you've provided

Improve test coverage and test directly LDContextBuilder.php

Describe alternatives you've considered
This class was tested indirectly previously but contained logic that would benefit from testing directly.

Additional context

N/A

@ActuallyConnor ActuallyConnor requested a review from a team as a code owner October 3, 2024 16:50
@ActuallyConnor ActuallyConnor changed the title Adding tests for LDContextBuilder test: Adding tests for LDContextBuilder Oct 3, 2024
Copy link
Member

@keelerm84 keelerm84 left a comment

Choose a reason for hiding this comment

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

Thank you for your interest and in taking the time to contribute!

This class was tested indirectly previously but contained logic that would benefit from testing directly.

I'm not sure which tests you are referring to. If you look at LDContextTest.php, most of the tests interact with the context builder (via LDContext::builder). As a result, I think several of the tests in this PR are redundant with what we currently have.

However, there are a couple of tests which I think we did miss. If you want to move those tests into that file, I would be happy to include those contributions. I've tried to note which ones to keep and which to save.

LMK if you have any questions!

@ActuallyConnor
Copy link
Contributor Author

@keelerm84 I've applied the requested changes to this PR

@keelerm84 keelerm84 merged commit 5b25095 into launchdarkly:main Oct 7, 2024
7 checks passed
@keelerm84
Copy link
Member

Thank you for addressing this feedback, and once again, thank you for taking the time to contribute to this project!

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