prioritize l10n nightlies below everything, regardless of branch

RESOLVED FIXED

Status

defect
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: bhearsum, Assigned: massimo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
The trick here is I think it shouldn't be below our "play with me" branches, e.g. try, cedar, ash.

Otherwise we run the risk of never completing l10n in the time it takes us to trigger next set of nightlies, and lose lots of testing on our localized jobs.
(Assignee)

Comment 2

5 years ago
This patch introduces some changes in the builder priority sorting function and a test suite to check the priority of different builders/requests. As they are now, the patch passes all the tests but I am not sure I have touched all the possible builders/priorities scenarios
Attachment #8395859 - Flags: feedback?(catlee)
(Assignee)

Updated

5 years ago
Assignee: nobody → mgervasini
(Assignee)

Comment 3

5 years ago
Added extra tests for l10n builders
Attachment #8395859 - Attachment is obsolete: true
Attachment #8395859 - Flags: feedback?(catlee)
Attachment #8396343 - Flags: review?(catlee)
(Assignee)

Comment 4

5 years ago
minor tests refactoring; fixed missing () around multiple exceptions in builderPriority(...)
Attachment #8396343 - Attachment is obsolete: true
Attachment #8396343 - Flags: review?(catlee)
Attachment #8396455 - Flags: feedback?(catlee)
(Assignee)

Comment 5

5 years ago
* builderPriority() returns a 3 element tuple: release, build_request_branch and submitted_at. (build_request_branch is: branch priority * 5 + request priority + builder priority)
* Updated tests
* run_test_suite.sh is still there because test-master.sh requires "/dev/shm"
Attachment #8396455 - Attachment is obsolete: true
Attachment #8396455 - Flags: feedback?(catlee)
Attachment #8397771 - Flags: review?
Attachment #8397771 - Flags: feedback?(catlee)
(Assignee)

Updated

5 years ago
Attachment #8397771 - Flags: review?
Comment on attachment 8397771 [details] [diff] [review]
[buildbot-configs] updated priorityBuilder and added tests4.patch

Review of attachment 8397771 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozilla/master_common.py
@@ +71,5 @@
>  def builderPriority(builder, request):
>      """
>      Our builder sorting function
> +    Returns (release, magic_number, submitted_at)
> +    where magic_number = 5 * branch_priority + req_priority + builder_priority

maybe want to clarify here that req_priority is actually the negative of the priority in the DB?

@@ +101,3 @@
>  
>      # Default builder priority is 100
> +    builder_priority = 1

comment needs updating. let's change the default to 0 while we're at it.

::: mozilla/test/test_builderPriority.py
@@ +220,5 @@
> +        priority_2 = l10n_builder.get_priority_request(high_priority_request)
> +        expected = [priority_0, priority_1, priority_2]
> +        import random
> +        # shuffle elements
> +        result = sorted(expected, key=lambda *args: random.random())

I don't think this is a good way to test that they get ordered properly. It allows for some indeterminism in the tests in the case we break the sorting function later.

Better to put the result in an order you don't expect, and then sort it, and make sure it matches your expected result. e.g.
result = sorted(reversed(expected))
self.assertTrue(result == expected)
Attachment #8397771 - Flags: feedback?(catlee) → feedback+
(Assignee)

Comment 7

5 years ago
Thanks for the feedback, catlee

this new patch should fix all the issues you have found.
This is still a feedback request because I have removed the run-test-suite.sh file (was added in a previous patch) and updated test-masters.sh so when executing:

test-masters.sh run-tests

it only runs the trial test function skipping all master tests part
Attachment #8397771 - Attachment is obsolete: true
Attachment #8398467 - Flags: feedback?(catlee)
Comment on attachment 8398467 [details] [diff] [review]
[buildbot-configs] updated priorityBuilder and added tests5.patch

Review of attachment 8398467 [details] [diff] [review]:
-----------------------------------------------------------------

::: test-masters.sh
@@ +41,5 @@
> +}
> +
> +if [ "$1" == "run-tests" ]
> +then
> +    # run trial and exist

exit!
Attachment #8398467 - Flags: feedback?(catlee) → feedback+
(Assignee)

Comment 9

5 years ago
Comment on attachment 8398467 [details] [diff] [review]
[buildbot-configs] updated priorityBuilder and added tests5.patch

got r+ from catlee. Landed
Attachment #8398467 - Flags: checked-in+
in production.
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Component: General Automation → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.