diff --git a/.github/workflows/mt.yml b/.github/workflows/mt.yml index 18ea155..88138f1 100644 --- a/.github/workflows/mt.yml +++ b/.github/workflows/mt.yml @@ -55,4 +55,4 @@ jobs: if: github.event_name == 'pull_request' run: | git fetch --depth=1 origin $GITHUB_BASE_REF - vendor/bin/infection --coverage=build/logs --threads=$(nproc) --show-mutations --no-interaction --only-covered --only-covering-test-cases --skip-initial-tests --git-diff-lines --git-diff-base=origin/$GITHUB_BASE_REF + vendor/bin/infection --coverage=build/logs --threads=$(nproc) --show-mutations --no-interaction --only-covering-test-cases --only-covering-test-cases --skip-initial-tests --git-diff-lines --git-diff-base=origin/$GITHUB_BASE_REF diff --git a/Makefile b/Makefile index c188a07..4f64d86 100644 --- a/Makefile +++ b/Makefile @@ -35,7 +35,7 @@ COMPOSER=$(PHP) $(shell which composer) INFECTION=vendor/bin/infection MIN_MSI=50 MIN_COVERED_MSI=50 -INFECTION_ARGS=--min-msi=$(MIN_MSI) --min-covered-msi=$(MIN_COVERED_MSI) --threads=$(JOBS) --coverage=build/logs --show-mutations --no-interaction --only-covered --only-covering-test-cases +INFECTION_ARGS=--min-msi=$(MIN_MSI) --min-covered-msi=$(MIN_COVERED_MSI) --threads=$(JOBS) --coverage=build/logs --show-mutations --no-interaction --only-covering-test-cases --only-covering-test-cases all: test diff --git a/lib/Tumblr/StreamBuilder/StreamRankers/CappedPostRanker.php b/lib/Tumblr/StreamBuilder/StreamRankers/CappedPostRanker.php index c62407b..f0fb0e3 100644 --- a/lib/Tumblr/StreamBuilder/StreamRankers/CappedPostRanker.php +++ b/lib/Tumblr/StreamBuilder/StreamRankers/CappedPostRanker.php @@ -61,20 +61,20 @@ class CappedPostRanker extends StreamRanker * CappedPostRanker constructor. * @param User $user User * @param string $identity The identity of this ranker - * @param bool $debug Need extra debug infos or not. e.g. Ranking score * @param bool $cap_desc Whether we are looking for the first violated blog post in descending order (top-down) * @param int $cap The cap applied (higher values are making the reranking less strict) * @param string $ranking_context The context that ranking is being applied to e.g. dashboard * @param bool $panel_allow_ranking Whether the meta key on panel regarding ranking is enabled or not + * @param bool $debug Need extra debug infos or not. e.g. Ranking score */ public function __construct( User $user, string $identity, - ?bool $debug = null, bool $cap_desc, int $cap, string $ranking_context, - bool $panel_allow_ranking + bool $panel_allow_ranking, + ?bool $debug = null ) { parent::__construct($identity); $this->user = $user; @@ -115,11 +115,11 @@ public static function from_template(StreamContext $context) return new self( $context->get_meta_by_key('user'), $context->get_current_identity(), - $context->get_meta_by_key('client_meta')->is_panel(), $context->get_optional_property('cap_desc', true), $context->get_optional_property('cap', 2), $context->get_optional_property('ranking_context', 'dashboard'), $context->get_meta_by_key('allow_ranking'), + $context->get_meta_by_key('client_meta')->is_panel() ); } diff --git a/tests/unit/Tumblr/StreamBuilder/StreamRankers/CappedPostRankerTest.php b/tests/unit/Tumblr/StreamBuilder/StreamRankers/CappedPostRankerTest.php index 7c6dd46..6ed4135 100644 --- a/tests/unit/Tumblr/StreamBuilder/StreamRankers/CappedPostRankerTest.php +++ b/tests/unit/Tumblr/StreamBuilder/StreamRankers/CappedPostRankerTest.php @@ -76,12 +76,12 @@ protected function setUp(): void $this->invalid_stream_elements = $this->setup_capped_invalid_stream_elements($blog_ids); $this->enabled_ranker_instance = $this->getMockBuilder(CappedPostRanker::class) - ->setConstructorArgs([$this->mock_user, $this->identity, false, true, 2, 'dashboard', false]) + ->setConstructorArgs([$this->mock_user, $this->identity, true, 2, 'dashboard', false, false]) ->getMock(); $this->enabled_ranker_instance->method('can_rank')->willReturn(true); $this->disabled_ranker_instance = $this->getMockBuilder(CappedPostRanker::class) - ->setConstructorArgs([$this->mock_user, $this->identity, false, true, 2, 'dashboard', false]) + ->setConstructorArgs([$this->mock_user, $this->identity, true, 2, 'dashboard', false, false]) ->getMock(); $this->disabled_ranker_instance->method('can_rank')->willReturn(false); } @@ -165,7 +165,7 @@ public function test_disabled_rank(): void public function test_invalid_elements(): void { $this->expectException(TypeMismatchException::class); - $this->invokeMethod($this->enabled_ranker_instance, 'pre_fetch', $this->invalid_stream_elements); + $this->invokeMethod($this->enabled_ranker_instance, 'pre_fetch', [$this->invalid_stream_elements]); } /** @@ -176,7 +176,7 @@ public function test_get_blog_dictionaries(): void $dictionaries = $this->invokeMethod( $this->enabled_ranker_instance, 'get_blog_dictionaries', - $this->stream_elements + [$this->stream_elements] ); $post_to_element = $dictionaries[0]; $blog_to_posts_ids = $dictionaries[1]; @@ -186,28 +186,101 @@ public function test_get_blog_dictionaries(): void $this->assertSame($post_id, $original_element->get_post_id()); } # Which posts belong to which blogs - $expected_blog_to_posts_ids = [111 => [1, 2, 3, 4, 5], 222 => [6, 7], 333 => [8]]; - $this->assertEquals($blog_to_posts_ids, $expected_blog_to_posts_ids); + $expected_blog_to_posts_ids = [111 => ['1', '2', '3', '4', '5'], 222 => ['6', '7'], 333 => ['8']]; + $this->assertSame($expected_blog_to_posts_ids, $blog_to_posts_ids); $expected_stats_per_blog = [111 => [5, 0], 222 => [2, 0], 333 => [1, 0]]; - $this->assertEquals($stats_per_blog, $expected_stats_per_blog); + $this->assertSame($expected_stats_per_blog, $stats_per_blog); + } + + /** + * Test that invalidate_post_id properly removes posts from blog_to_posts_ids + */ + public function test_invalidate_post_id_removes_posts(): void + { + $ranker = new CappedPostRanker($this->mock_user, $this->identity, true, 2, 'dashboard', false); + + // Get initial blog dictionaries using reflection + $dictionaries = $this->invokeMethod($ranker, 'get_blog_dictionaries', [$this->stream_elements]); + $blog_to_posts_ids = $dictionaries[1]; + + // Verify initial state + $expected_initial = [111 => ['1', '2', '3', '4', '5'], 222 => ['6', '7'], 333 => ['8']]; + $expected_post_counts = [111 => [5, 0], 222 => [2, 0], 333 => [1, 0]]; + $this->assertSame($expected_initial, $blog_to_posts_ids); + $this->assertSame($expected_post_counts, $dictionaries[2]); + + // Test invalidating a specific post using reflection + $violated_post_id = $this->invokeMethod($ranker, 'invalidate_post_id', ['111', '3', &$blog_to_posts_ids]); + // The method returns null when a specific post ID is provided, but the post should be removed + $this->assertNull($violated_post_id); + + // Verify post was removed from blog 111 + $this->assertCount(4, $blog_to_posts_ids[111]); // Should have 4 posts instead of 5 + $this->assertContains('1', $blog_to_posts_ids[111]); + $this->assertContains('2', $blog_to_posts_ids[111]); + $this->assertContains('4', $blog_to_posts_ids[111]); + $this->assertContains('5', $blog_to_posts_ids[111]); + } + + /** + * Test that invalidate_post_id returns the first available post when no specific post is provided + */ + public function test_invalidate_post_id_returns_first_available(): void + { + $ranker = new CappedPostRanker($this->mock_user, $this->identity, true, 2, 'dashboard', false); + + $dictionaries = $this->invokeMethod($ranker, 'get_blog_dictionaries', [$this->stream_elements]); + $blog_to_posts_ids = $dictionaries[1]; + + // Test getting first available post from blog 111 using reflection + $first_post = $this->invokeMethod($ranker, 'invalidate_post_id', ['111', null, &$blog_to_posts_ids]); + $this->assertSame(1, $first_post); // Returns integer, not string + + // Verify first post was removed + $this->assertNotContains('1', $blog_to_posts_ids[111]); + $this->assertCount(4, $blog_to_posts_ids[111]); + $this->assertContains('2', $blog_to_posts_ids[111]); + $this->assertContains('3', $blog_to_posts_ids[111]); + $this->assertContains('4', $blog_to_posts_ids[111]); + $this->assertContains('5', $blog_to_posts_ids[111]); + } + + /** + * Test that invalidate_post_id handles non-existent posts gracefully + */ + public function test_invalidate_post_id_nonexistent_post(): void + { + $ranker = new CappedPostRanker($this->mock_user, $this->identity, true, 2, 'dashboard', false, false); + + $dictionaries = $this->invokeMethod($ranker, 'get_blog_dictionaries', [$this->stream_elements]); + $blog_to_posts_ids = $dictionaries[1]; + + // Try to invalidate a non-existent post using reflection + $result = $this->invokeMethod($ranker, 'invalidate_post_id', ['111', '999', &$blog_to_posts_ids]); + $this->assertNull($result); + + // Verify no changes were made + $this->assertCount(5, $blog_to_posts_ids[111]); + $this->assertContains('1', $blog_to_posts_ids[111]); + $this->assertContains('2', $blog_to_posts_ids[111]); + $this->assertContains('3', $blog_to_posts_ids[111]); + $this->assertContains('4', $blog_to_posts_ids[111]); + $this->assertContains('5', $blog_to_posts_ids[111]); } /** * Helper method to enable testing private and protected methods - * @param object $object The object that the private method belongs to + * @param CappedPostRanker $object The object that the private method belongs to * @param string $method_name The name of the method we want to test * @param array $parameters List of parameters passed to the method * @return mixed Whatever the method would return * @throws \ReflectionException Something went wrong with reflection */ - public function invokeMethod(object &$object, string $method_name, array $parameters = []) + public function invokeMethod(CappedPostRanker &$object, string $method_name, array $parameters = []) { - $reflection = new \ReflectionClass(get_class($object)); + $reflection = new \ReflectionClass(CappedPostRanker::class); $method = $reflection->getMethod($method_name); $method->setAccessible(true); - if ($method_name = 'get_blog_dictionaries') { - $parameters = [$parameters]; - } return $method->invokeArgs($object, $parameters); } }