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

Tests For Channel Provider Classes #2644

Merged
merged 6 commits into from Dec 8, 2022

Conversation

skellamp
Copy link
Contributor

@skellamp skellamp commented Dec 4, 2022

No description provided.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks for this, @skellamp! I've left a few suggestions on the AlphaBrowseTest -- mostly about clarifying naming/documentation, and also pointing out some PHP language features that might be helpful if you are not already familiar with them (though if you simply prefer not to use them, feel free to ignore those suggestions). I think that some of my feedback probably applies across the other tests as well, so I'm going to let you adjust the first one (and make equivalent changes to the other) before taking the time to do a deep review of everything. Please let me know when you're ready for me to take another look.

@skellamp skellamp marked this pull request as ready for review December 6, 2022 20:10
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Thanks, @skellamp -- looking even better; just a few more minor comments.

public function testGetFromRecord(): void
{
$recordDriver = $this->getDriver(['solrField' => 'foo']);
[$alpha, $expetedResult] = $this->configureTestTargetAndExpectations();
Copy link
Member

Choose a reason for hiding this comment

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

Typo: should be $expectedResult. (Looks like this is copied and pasted in a few places -- hopefully a search-and-replace can easily fix it :-) ).

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Looking good, @skellamp -- thanks again! Merging now.

@demiankatz demiankatz merged commit 7426045 into vufind-org:dev Dec 8, 2022
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Aug 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
2 participants