AdamWill
commented
3 months ago Oh, one of the commit messages promises my evaluation of the test_approve_testing tests as a comment on the PR, soo:
assessment of approve_testing code
there are only four main paths.
- if not update.release.mandatory_days_in_testing and not update.autotime: bail
- if not update.meets_testing_requirements: bail
- if not update.autotime or update.days_in_testing < update.stable_days (but does meet testing requirements): 3a) comment + publish (if not already done) 3b) bail (if already commented)
- if update.autotime and update.days_in_testing >= update.stable_days (and meets testing requirements): 4a) set_request stable (if update.release.composed_by_bodhi) 4b) actually push update stable (if not update.release.composed_by_bodhi)
assessment of test_approve_testing tests
test_autokarma_update_meeting_time_requirements_gets_one_comment - ensures we comment exactly once on multiple runs when update meets manual push time threshold but not autopush threshold test_non_autokarma_critpath_update_meeting_karma_requirements_gets_one_comment - ditto, but for a non-autokarma, critpath update, and tries to meet the karma threshold. sets update.stable_karma=1, then adds 1 karma before 'approving' (which I think means the update actually hits +2) test_non_autokarma_update_meeting_karma_requirements_gets_one_comment - ditto, non-autokarma, non-critpath test_autotime_update_no_autokarma_met_karma_requirements_get_comments - ditto, only update has autotime=True. doesn't check for repeat comments test_non_autokarma_critpath_update_meeting_time_requirements_gets_one_comment - ditto, non-autokarma, critpath, sets mandatory time in testing to 14 days and time in testing to 14 days. intends to check that the message doesn't say "7 days" if the minimum is not 7 days. current message is very generic ("This update can be pushed to stable now if the maintainer wishes") so this test is less relevant test_non_autokarma_update_with_unmet_karma_requirement_after_time_met - similar, but doesn't check for only one comment. non-autokarma, non-critpath, update is set to meet time in testing requirement but no karma threshold.
test_autokarma_update_without_mandatory_days_in_testing - tests we dip early if no mandatory_days_in_testing or autotime test_non_autokarma_update_without_mandatory_days_in_testing - ditto, non-autokarma
test_autokarma_update_not_meeting_testing_requirements - ensures we don't comment on or approve update that doesn't meet requirements. leaves karma at default (I think +1), sets days in testing to 6 test_non_autokarma_critpath_update_not_meeting_time_requirements_gets_no_comment - similar, but non-autokarma, critpath. doesn't check date_approved. sets stable_karma = 1, leaves karma at default (+1). intends to test the case where update meets stable_karma but autokarma is not set and update doesn't reach critpath_min_karma test_non_autokarma_update_with_unmet_karma_requirement - ditto, non-autokarma, not critpath. update is set not to meet any threshold at all (time or karma, auto or manual)
test_exception_handler - tests exception handler test_exception_handler_on_the_second_update - moar exception handler
test_subsequent_comments_after_initial_push_comment - tests the case where the update is approved, edited, then approved again, expects to see two approvals. is effectively redundant with test_has_stable_comment tests in test_models, because all approve_testing cares about is whether the update has_stable_comment
test_autotime_update_meeting_test_requirements_gets_pushed - tests non-autokarma, non-critpath, autotime update with stable_days set to 7 and time in testing of 7 days is autopushed (and marked approved). does not check whether an approval comment is posted test_autotime_update_does_not_meet_stable_days_doesnt_get_pushed - similar, but sets stable_days to 10 and time in testing to 7 (so autotime requirement is not met) and tests the update is not pushed test_autotime_update_meeting_stable_days_get_pushed - similar, but sets both stable_days and time_in_testing to 10. functionally equivalent to test_autotime_update_meeting_test_requirements_gets_pushed with current code, not sure whether this is ever useful test_autotime_update_no_autokarma_met_karma_and_time_requirements_get_pushed - similar, but update also meets all karma requirements (but has autokarma disabled). probably pointless test_autotime_update_with_autokarma_met_karma_and_time_requirements_get_pushed - ditto, but update has autokarma enabled. probably isn't actually testing approve_testing at all; update would be pushed by check_karma_thresholds() when the +1 karma is posted. test passes if you skip approve_testing_main(). should be a check_karma_thresholds() test
test_no_autotime_update_meeting_stable_days_and_test_requirement - tests update without autotime set does not get autopushed, even with a stable_days value set and reached (uses 10 days)
test_autotime_update_does_not_meet_test_requirements - tests autotime update with stable_days and mandatory_days_in_testing set to 2 is not pushed if days in testing is 1. mostly functionally equivalent to test_autotime_update_does_not_meet_stable_days_doesnt_get_pushed , except with a non-default mandatory days in testing value and an update that does not meet it. probably useless
test_autotime_update_does_no_mandatory_days_in_testing - tests that autotime update is pushed stable with zero days in testing if stable_days is 0. relies on the default value of stable_days being 0, but doesn't say so. sets mandatory_days_in_testing to 0 to ensure meets_testing_requirements passes test_autotime_update_zero_day_in_testing_meeting_test_requirements_gets_pushed - exactly the same, but explicitly sets stable_days to 0 test_autotime_update_zero_day_in_testing_no_gated_gets_pushed_to_rawhide - ditto, but for release not composed by rawhide. tests the path where approve_testing actually has to do all the push work itself. parametrized to also test when update is from a side tag test_autotime_update_zero_day_in_testing_fail_gating_is_not_pushed - ditto test_autotime_update_zero_day_in_testing_meeting_test_requirements_gets_pushed, buts sets test_gating_status to failed so meets_testing_requirements will fail, and expects update not to be pushed test_autotime_update_negative_karma_does_not_get_pushed - ditto test_autotime_update_zero_day_in_testing_meeting_test_requirements_gets_pushed , but adds a negative karma. is actually testing that update.comment() disables autotime, though this is not clear. should be a check_karma_thresholds() test test_autotime_update_no_autokarma_negative_karma_not_pushed - virtually identical to test_autotime_update_negative_karma_does_not_get_pushed (both updates have autokarma = False), but sets stable_karma to 10 instead of 1 (seems irrelevant). does assert that update.autotime = False, so this test realized to some extent what was actually happening. should be check_karma_thresholds() test
test_update_conflicting_build_not_pushed - tests we don't push updates with conflicting builds. also tests we don't post a comment saying they're approved, or set date_approved.
test_autotime_update_gets_pushed_dont_send_duplicated_notification - tests we don't publish UpdateRequirementsMetStable again when auto-pushing, if the update previously met the manual push threshold and had it published then. doesn't test whether we repeat the comment test_autotime_update_with_stable_comment_set_stable_on_branched - tests similar flow, but for the not-composed-by-bodhi case where approve_testing actually does the work
ACTUALLY USEFUL TESTS:
- two early bails (until we remove the first)
- when meets_testing_requirements but not autotime push requirements: "ready for stable" comment, publish UpdateRequirementsMetStable, set date_approved, if not already done. do not push. parametrize: composed_by_bodhi, side_tag. check all possible ways of not meeting autotime requirements
- when meets_testing_requirements and autotime push requirements: set request (composed-by-bodhi) or do all the work (not-composed-by-bodhi). check everything is done in both cases. check we do not post the "ready for stable" comment but do publish UpdateRequirementsMetStable if the update does not have the comment.
- conflicting builds checks. note only done when autopushing on not-composed-by-bodhi path, so we still set date_approved, post "ready for stable" and/or publish UpdateRequirementsMetStable on other paths even if update has conflicting builds
- exception handler checks
- check against repeated comments
STUFF TO ENSURE IS TESTED ELSEWHERE:
- different ways of meeting different thresholds (these should all be on the tests for meets_testing_requirements / check_karma_thresholds)
- whether check_karma_thresholds disables autopush on negative karma (should be in its own tests)
- autopush of autokarma-enabled update for composed-by-bodhi release reaching stable_karma (this is done by check_karma_thresholds)
mattiaverga
This is a huge patch series that more or less entirely overhauls Bodhi's approach to checking if updates "meet requirements" in various senses. See each individual commit's extensive message for details, but broadly the goal is to make the code (and tests) more consistent, correct, in line with the actual Fedora update policies, and simpler.