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

[Fail Case] Add test for aggregator transforms #162

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hailwood
Copy link

@hailwood hailwood commented Mar 19, 2019

@nunomaduro this is the failing test for #158

You'll note that we've setup an extended "Thread" model where the toSearchableArray method specifies to only use the date transform.

The testTransformsAreRespectedOnSingleModel passes, I've only added it to ensure this is tested in the future.

The testModelTransformsAreRespectedOnAggregators test then fails, as while the following check passes on the single model, it fails on the aggregate as the '100' has been cast to 100.

return $argument[0]['subscriber_count'] === '100';

The test can be made to pass by adding the following function to our ThreadAggregatorUsingOnlyDateTransform class as this causes the check at https://github.com/algolia/scout-extended/blob/master/src/Jobs/UpdateJob.php#L113 to pass and as such we don't call the default transformers again, however I feel a better fix would be an update in the hasToSearchableArray method on UpdateJobs`.

public function toSearchableArray(): array
{
  return parent::toSearchableArray();
}

P.S. sorry about the non-matching code style. I just quickly wrote this up to make the point clear.

@hailwood hailwood changed the title Add test for aggregator transforms [Fail Case] Add test for aggregator transforms Mar 19, 2019
@nunomaduro nunomaduro added the bug Something isn't working label Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants