Limit test predictions for DirectMapping strategy
From Draft to Ready
-
!45 (merged) is merged -
The target branch is master
Context
Closes gitlab-org/gitlab#450374 (closed)
What's in this MR?
[Feature] Add limit_percentage and limit_min options for DirectMatching
Merge request reports
Activity
changed milestone to %16.10
added Engineering Productivity maintenanceperformance + 1 deleted label
assigned to @ddieulivol
1 Warning 
3dc995a7: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not automatically notify them for you.
Reviewer Maintainer @nao.hashizume
(UTC-7, 8 hours behind author)
@splattael (UTC+1, same timezone as author)
If needed, you can retry the
danger-reviewjob that generated this comment.Generated by
- A deleted user
added bugperformance typebug labels
removed bugperformance typebug labels
- A deleted user
added bugperformance typebug labels
removed bugperformance typebug labels
mentioned in issue gitlab-org/gitlab#450374 (closed)
Hey @godfat-gitlab, Could you please review this MR? Thanks in advance
In particular, I'd like your feedback on the second commit, which contains the real change for this MR (the first commit is a refactoring). You're more than welcome to review the entire MR of course.
requested review from @godfat-gitlab
changed milestone to %16.11
-
@ddieulivol Thanks! Given this is still in draft and there are things can potentially move, I only reviewed the overall concept. I think it's a great idea! The default should probably be
randomon the other hand.In the future, if we want to make this reproducible, we might want to use a new random object, too. (
Random.new, so it doesn't share the seed with the global one, and we can provide the seed independently)I really like the idea of using a minimal and percentage, too. Perhaps we don't have to provide the
firstoption, because I don't think that's a good idea anyway
Edited by Lin Jen-Shin removed review request for @godfat-gitlab
added 5 commits
-
d4b82360...6279fb0b - 2 commits from branch
master - c9c41b58 - Go from class methods to instance methods
- b438f1e2 - Add limit_per_file and limit_method options
- e2299f47 - Use limit_percentage and limit_min as args
Toggle commit list-
d4b82360...6279fb0b - 2 commits from branch
- A deleted user
added bugperformance typebug labels
removed maintenanceperformance label
added 1 commit
- a2d755d1 - Reintroduce backwards-compatible class methods
marked the checklist item !45 (merged) is merged as completed
added 1 commit
- 5c547def - Reintroduce backwards-compatible class methods
-
Thank you for your feedback @godfat-gitlab . Could you please review this MR again?
Since your last review, I did the following:
- Reintroduced class methods, and separated validation from instantiation
- Added
:limit_percentageand:limit_minarguments for DirectMatching, as discussed in gitlab-org/gitlab#450374 (comment 1817102957)
requested review from @godfat-gitlab
mentioned in issue #27
- Resolved by Lin Jen-Shin
@godfat-gitlab FYI - According to reviewer roulette, you are a project maintainer, but it seems that you do not have merge rights?
- Last reply by Lin Jen-Shin
-
@ddieulivol Thanks! I have one suggestion around using an integer instead of using a floating point, and one suggestion to see if we can remove stubs around complex stubs. If not, that's fine!
removed review request for @godfat-gitlab
-
Thank you for your feedback @godfat-gitlab. Could you please review this MR again?
Since your last review, I did the following:
Edited by David Dieulivol requested review from @godfat-gitlab
added 1 commit
- d20ad4dd - Only allow integer values for limit_percentage
- Resolved by David Dieulivol
@ddieulivol Thank you, looks good to me!
Oh well, I don't believe it's typebug
- Last reply by David Dieulivol
mentioned in commit 94785f57
added featureenhancement label
added typefeature label and removed bugperformance typebug labels
mentioned in merge request !49 (merged)
mentioned in merge request !52 (merged)
34 return if limit_min_valid 35 36 raise "Invalid value for limit_min: should be an integer strictly greater than zero" 37 end 38 39 def initialize(map, limit_percentage: nil, limit_min: nil) 40 @map = map 41 @limit_percentage = limit_percentage 42 @limit_min = limit_min 43 end 27 44 28 45 def match(files) 29 46 Array(files).inject(Set.new) do |result, file| 30 47 test_files = @map.fetch(file, []) 48 49 if limit_percentage -
note (non-blocking): There is an issue below, where we still consider
limit_percentageto be a number between 0 and 1 (where it's now a number between 1 and 100!)Fix in !52 (merged)
mentioned in merge request gitlab-org/gitlab!148167 (merged)
mentioned in issue gitlab-org/gitlab#413510 (closed)
