feat: add bonuses to the points system#1
Conversation
|
This is a benchmark review for experiment This pull request was cloned from Experiment configurationreview_config:
# User configuration for the review
# - benchmark - use the user config from the benchmark reviews
# - <value> - use the value directly
user_review_config:
enable_ai_review: true
enable_rule_comments: false
enable_complexity_comments: benchmark
enable_security_comments: benchmark
enable_tests_comments: benchmark
enable_comment_suggestions: benchmark
enable_pull_request_summary: benchmark
enable_review_guide: benchmark
enable_approvals: false
base_branches: [base-sha.*]
ai_review_config:
# The model responses to use for the experiment
# - benchmark - use the model responses from the benchmark reviews
# - llm - call the language model to generate responses
model_responses:
comments_model: benchmark
comment_validation_model: benchmark
comment_suggestion_model: benchmark
complexity_model: benchmark
security_model: benchmark
tests_model: benchmark
pull_request_summary_model: benchmark
review_guide_model: benchmark
overall_comments_model: benchmark
# The pull request dataset to run the experiment on
pull_request_dataset:
# CodeRabbit
- https://github.com/neerajkumar161/node-coveralls-integration/pull/5
- https://github.com/gunner95/vertx-rest/pull/1
- https://github.com/Altinn/altinn-access-management-frontend/pull/1427
- https://github.com/theMr17/github-notifier/pull/14
- https://github.com/bearycool11/AI_memory_Loops/pull/142
# Greptile
- https://github.com/gumloop/guMCP/pull/119
- https://github.com/autoblocksai/python-sdk/pull/335
- https://github.com/grepdemos/ImageSharp/pull/6
- https://github.com/grepdemos/server/pull/61
- https://github.com/websentry-ai/pipelines/pull/25
# Graphite
- https://github.com/KittyCAD/modeling-app/pull/6648
- https://github.com/KittyCAD/modeling-app/pull/6628
- https://github.com/Varedis-Org/AI-Test-Repo/pull/2
- https://github.com/deeep-network/bedrock/pull/198
- https://github.com/Metta-AI/metta/pull/277
# Copilot
- https://github.com/hmcts/rpx-xui-webapp/pull/4438
- https://github.com/ganchdev/quez/pull/104
- https://github.com/xbcsmith/ymlfxr/pull/13
- https://github.com/tinapayy/B-1N1T/pull/36
- https://github.com/coder/devcontainer-features/pull/6
# Questions to ask to label the review comments
review_comment_labels: []
# - label: correct
# question: Is this comment correct?
# Benchmark reviews generated by running
# python -m scripts.experiment benchmark <experiment_name>
benchmark_reviews: []
|
Reviewer's GuideThis PR implements speed and streak bonuses by adding File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Hellebore
left a comment
Hellebore
left a comment
There was a problem hiding this comment.
Hey @Hellebore - I've reviewed your changes - here's some feedback:
- Consider extracting the point calculation and player rewarding logic from the
GameQuestionbefore_savecallback into a dedicated service object to simplify the model's responsibilities. - Extract magic numbers used for time thresholds and bonus points in
calculate_speed_bonus, and the multiplier incalculate_streak_bonus, into constants for improved readability and maintainability.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 7 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| all_players = game.game_players.includes(:player_answers) | ||
| all_players.find_each do |player| | ||
| current_question_answer = player.player_answers.find_by(game_question_id: id) | ||
| unless current_question_answer |
There was a problem hiding this comment.
suggestion: Use of -1 as a sentinel value in player_answers creation.
Extract the -1 sentinel into a named constant and verify downstream logic handles it correctly.
Suggested implementation:
class GameQuestion < ApplicationRecord
INVALID_ANSWER_SENTINEL = -1 player.player_answers.create(game_question: self, answer_id: GameQuestion::INVALID_ANSWER_SENTINEL, correct: false)Ensure all references to the sentinel value in related downstream logic now use GameQuestion::INVALID_ANSWER_SENTINEL instead of the literal -1.
| assert_nil @game_player.find_answer_for(game_question) | ||
| end | ||
|
|
||
| # award_points |
There was a problem hiding this comment.
suggestion (testing): Missing test coverage for combined speed and streak bonuses in award_points!.
Add tests for a player with a non-trivial streak (e.g., current_streak > 1 or mocked streak_length) and verify that award_points! includes base points, the speed bonus, and the streak bonus.
Suggested implementation:
test "awards only question points" do
@game_player.update!(points: 10)
@game_player.award_points!(10, 100) # too late = 0 bonus
assert_equal 20, @game_player.reload.points
end
test "awards combined speed and streak bonuses" do
# Setup a non-trivial streak scenario by setting current_streak > 1
@game_player.update!(points: 100, current_streak: 3)
base_points = 10
fast_answer_time = 5 # assumed fast enough to trigger a speed bonus
@game_player.award_points!(base_points, fast_answer_time)
updated_points = @game_player.reload.points
# The total points should be greater than the sum of base points plus the initial points,
# indicating that the speed and streak bonuses were applied.
assert_operator updated_points, :>, 100 + base_points, "Expected bonus points for quick answer with a streak"
endIf your game player model uses a different attribute for tracking the streak (for example, streak_length), adjust the test accordingly. Also, if there is a well-defined bonus formula, you may want to calculate the expected total points exactly instead of using a greater-than assertion.
| test "awards zero points when both are zero" do | ||
| @game_player.update!(points: 10) | ||
| @game_player.award_points!(1, 30) # too late = 0 bonus | ||
| assert_equal 11, @game_player.reload.points |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for award_points! where time_taken is nil.
Check that award_points! with time_taken set to nil applies no speed bonus while still granting question and streak points.
Suggested implementation:
# award_points
test "awards zero points when both are zero" do
@game_player.update!(points: 10)
@game_player.award_points!(1, 30) # too late = 0 bonus
assert_equal 11, @game_player.reload.points
end
test "awards base and streak points without speed bonus when time_taken is nil" do
@game_player.update!(points: 10)
@game_player.award_points!(1, nil)
assert_equal 11, @game_player.reload.points
endThe new test case assumes that when time_taken is nil, the method award_points! grants only the base (and possibly streak) points without any speed bonus. Adjust the expected points accordingly if your game's logic requires adding additional streak points.
| test "zero-point question" do | ||
| bonus = @game_player.send(:calculate_speed_bonus, 1) | ||
| assert_equal 3, bonus # falls under low-point logic |
There was a problem hiding this comment.
nitpick (testing): Test names for calculate_speed_bonus related to question points are potentially misleading.
Rename tests to reflect that calculate_speed_bonus depends only on time, e.g. "calculates bonus correctly regardless of question points."
|
|
||
| # Private methods | ||
|
|
||
| # calculate_speed_bonus, |
There was a problem hiding this comment.
suggestion (testing): Missing test case for calculate_speed_bonus with time_taken = nil.
Add a test that passes nil to calculate_speed_bonus and asserts it returns 0.
| test "reward_players_if_completed awards points to correct answers and resets streaks for incorrect or no answers" do | ||
| @game_question.answering! | ||
| @game = @game_question.game | ||
| @question = @game_question.question | ||
|
|
||
| # Create players | ||
| @player1 = game_players(:one) | ||
| @player2 = game_players(:two) | ||
| @player3 = game_players(:three) # Player 3 does not answer | ||
|
|
| assert_equal "Answer submitted successfully", response_json["message"] | ||
| end |
There was a problem hiding this comment.
suggestion (testing): Controller test should assert attributes of the created PlayerAnswer.
Add assertions for the created PlayerAnswer’s correct attribute (matching the submitted answer) and ensure time_taken is populated (e.g., not nil or within an expected range).
| assert_equal "Answer submitted successfully", response_json["message"] | |
| end | |
| assert_equal "Answer submitted successfully", response_json["message"] | |
| player_answer = PlayerAnswer.last | |
| # Assert that the selected answer matches the submitted answer and is marked as correct | |
| assert_equal @correct_answer.id, player_answer.selected_answer_id | |
| assert player_answer.correct, "PlayerAnswer should be marked as correct" | |
| # Assert that the time_taken attribute is populated and greater than zero | |
| assert_not_nil player_answer.time_taken, "time_taken should be present" | |
| assert_operator player_answer.time_taken, :>, 0, "time_taken should be greater than zero" | |
| end |
|
|
||
| require "test_helper" | ||
|
|
||
| class GamePlayerAnswersControllerTest < ActionDispatch::IntegrationTest |
There was a problem hiding this comment.
suggestion (testing): Consider explicitly testing that points are not awarded by the controller.
Add an assertion that @game_player.points remains unchanged immediately after the controller action to confirm it no longer updates points.
Suggested implementation:
setup do
@user = users(:one)
@game = games(:one)
@game_question = game_questions(:one)
@correct_answer = answers(:correct)
@incorrect_answer = answers(:incorrect)
@game_player = game_players(:one)
set_current_user(@user)
end
test "does not award points after controller action" do
initial_points = @game_player.points
# Assuming the answer submission is a POST request to a standard route.
# Adjust the URL and parameters as needed for your application.
post game_player_answers_url, params: {
game_player_answer: {
game_player_id: @game_player.id,
game_question_id: @game_question.id,
answer_id: @incorrect_answer.id
}
}
assert_equal initial_points, @game_player.reload.points, "Points should remain unchanged after controller action"
endIf your controller action or routes differ from the assumed ones, adjust the URL (game_player_answers_url) and the parameters accordingly. This test ensures that when the controller processes an answer, it does not update the points for the game player.
2 participants
Summary by Sourcery
Enhance the points system by introducing bonuses for player performance, including speed and streak bonuses, and improve game mechanics tracking
New Features:
Bug Fixes:
Enhancements:
Tests: