From 7ccf8d4584b217e043f74deaac1546681ad517fe Mon Sep 17 00:00:00 2001 From: DonalMe <76789979+DonalMe@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:56:18 -0400 Subject: [PATCH 1/6] Add a Slack module for posting messages from rules Nothing in bugbot posted to Slack before this. `bugbot/slack.py` adds the transport on its own, with no caller yet; the first one lands separately. It is one bot for the whole of bugbot. Everything about who is posting lives here -- the token it authenticates with and the name it appears under -- and a caller supplies only the message and where to send it. Where a given rule posts is that rule's business, kept wherever the rest of that rule's configuration is, so a second rule posting somewhere else needs no change here. Messages go through chat.postMessage. That needs a bot token carrying chat:write, and chat:write.public as well to reach a channel the bot has not been invited to. The token is the only secret involved and comes from `slack_bot_token` in configs/config.json or from `SLACK_ACCESS_TOKEN`. A channel ID is not a secret and so does not belong in that file. Messages are posted as "Firefox Release Management Bot". A Slack app's own name is set in its app configuration, is shared by everything the token posts, and is generally not what a reader of one of these messages should see. It is not overridable per call: there is one bot, so there is one name, and letting a rule pick its own would only make bugbot look like several senders. Sending it needs chat:write.customize on the token as well; Slack rejects the message when that scope is missing rather than dropping the name, so it cannot quietly stop working. chat.postMessage reports application errors as HTTP 200 with ok=false, so the body rather than the status is what has to be checked. On missing_scope it names both the scope it wanted and the ones the token carries, which the error passes along because it is very hard to act on otherwise. Nothing is retried, unlike reads: a POST that times out may well have arrived, and retrying it risks posting the message twice. --- README.rst | 5 +- bugbot/slack.py | 162 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_slack.py | 131 +++++++++++++++++++++++++++++++++++ 3 files changed, 297 insertions(+), 1 deletion(-) create mode 100644 bugbot/slack.py create mode 100644 tests/test_slack.py diff --git a/README.rst b/README.rst index 6e8ccded5..6c6545c0e 100644 --- a/README.rst +++ b/README.rst @@ -59,11 +59,14 @@ Before running: "iam_client_secret": "xxxxxxxxxxxxxx", "iam_client_id": "xxxxxxxxxxxxxx", "socorro_token": "xxxxxxxxxxxxxx", - "hackbot_api_key": "xxxxxxxxxxxxxx" + "hackbot_api_key": "xxxxxxxxxxxxxx", + "slack_bot_token": "xoxb-xxxxxxxxxxxxxx" } The ``hackbot_api_key`` is only needed by rules that start a `hackbot `_ agent run (currently ``frontend_triage``). Those rules talk to ``https://hackbot-api.moz.tools`` by default; set ``HACKBOT_API_URL`` to point at a different deployment. +The ``slack_bot_token`` is only needed by rules that post a message to Slack. It is a Slack app's Bot User OAuth Token and needs the ``chat:write``, ``chat:write.customize`` and ``chat:write.public`` scopes. + Do a dryrun:: uv run -m bugbot.rules.stalled diff --git a/bugbot/slack.py b/bugbot/slack.py new file mode 100644 index 000000000..f919d24de --- /dev/null +++ b/bugbot/slack.py @@ -0,0 +1,162 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +"""Post messages to Slack. + +One bot for the whole of bugbot. Everything about who is posting lives here -- +the token it authenticates with and the name it appears under -- and a caller +supplies only the message and where to send it. Where a rule posts is that +rule's to say, kept wherever the rest of its configuration is -- in the rule, or +in a module it shares with the other rules it posts alongside -- so a second +rule posting somewhere else needs no change in here. + +Messages go through chat.postMessage, which needs a bot token carrying +chat:write, and chat:write.public as well to post to a channel the bot has not +been invited to. One token serves every rule, so it is read from here rather +than passed in: it is a secret, and comes from `slack_bot_token` in +`configs/config.json` or from `SLACK_ACCESS_TOKEN`. + +A channel is an ID rather than a name -- a "C…" string, the last section of a +channel's 'copy link' URL, or "D…" for a DM. Unlike the token it is not a +secret, so it belongs with a rule's configuration and not in config.json. + +Messages are posted under `USERNAME` rather than whatever the Slack app happens +to be called, which needs `chat:write.customize` on the token as well. + +`SLACK_API_URL`, `SLACK_ACCESS_TOKEN` and the error wording follow taskcluster's +notify service (services/notify), which solves the same problem. `SLACK_API_URL` +exists to point at a test server, which is the only way to exercise any of this +without a real token. +""" + +import json +import os + +import requests + +from bugbot import utils + +TIMEOUT_SECONDS = 15 + +DEFAULT_API_URL = "https://slack.com/api/" +API_URL_VAR = "SLACK_API_URL" +TOKEN_VAR = "SLACK_ACCESS_TOKEN" + +# The key the bot token lives under in `configs/config.json`. Not required: a +# deployment that posts to no channel needs no token, so it is read with `.get` +# rather than validated at load time the way `bz_api_key` is. +TOKEN_KEY = "slack_bot_token" + +# The name every message is posted under. Not overridable: there is one bot, so +# there is one name, and a rule choosing its own would only make bugbot look like +# several senders. +# +# A Slack app's own name is set in its app configuration, is shared by everything +# the token posts, and is generally not what a reader of one of these messages +# should see. This is what they see instead. +# +# Sending it needs `chat:write.customize` on the token on top of `chat:write`. +# Slack rejects the message outright when that scope is missing rather than +# ignoring the name, so this is not something that quietly stops working. +USERNAME = "Firefox Release Management Bot" + + +def get_token() -> str: + """The bot token to post with. + + `SLACK_ACCESS_TOKEN` wins over `slack_bot_token` in `configs/config.json`. A + missing config file counts as a missing key rather than an error, so a + checkout with no credentials still imports. + + Raises rather than returning empty: these are cron jobs whose whole purpose is + the message, so a missing token has to stop the run and be seen. + """ + token = os.environ.get(TOKEN_VAR, "").strip() + if token: + return token + + try: + token = utils.get_login_info().get(TOKEN_KEY, "") + except OSError: + token = "" + + if not token: + raise RuntimeError( + f"Posting to Slack needs a bot token with the chat:write scope " + f"(chat:write.public to post without being invited), from {TOKEN_VAR} " + f"or {TOKEN_KEY} in configs/config.json" + ) + + return token + + +def post_to_slack( + channel: str, + text: str, + blocks: list[dict] | None = None, + thread_ts: str | None = None, +) -> str: + """Post a message to a Slack channel, and return its timestamp. + + `channel` is a channel ID; see the module docstring. Who the message comes + from is not a caller's concern: the token and the display name are this + module's, and every rule posts as the same bot. + + `text` is always sent: on a blocks message it is the notification and the + fallback for clients that can't render blocks. + + `thread_ts` replies in thread, and takes the timestamp this returns for an + earlier message. + + Link previews are always suppressed. These messages are notifications built + around their links, and an unfurl below one repeats what the message already + says at several times the height. + + Not retried, unlike reads: a POST that times out may well have arrived, so + retrying risks posting the message twice. A failure here fails the run + instead, which is visible in the error digest and harmless to repeat by hand. + """ + payload: dict = { + "channel": channel, + "text": text, + "username": USERNAME, + "unfurl_links": False, + "unfurl_media": False, + } + if blocks is not None: + payload["blocks"] = blocks + if thread_ts is not None: + payload["thread_ts"] = thread_ts + + api_url = (os.environ.get(API_URL_VAR) or DEFAULT_API_URL).rstrip("/") + + # The body is encoded here rather than passed as `json=` so the charset can be + # spelled out: Slack answers a bare application/json with a missing_charset + # warning. + response = requests.post( + f"{api_url}/chat.postMessage", + data=json.dumps(payload).encode("utf-8"), + headers={ + "Content-Type": "application/json; charset=utf-8", + "Authorization": f"Bearer {get_token()}", + }, + timeout=TIMEOUT_SECONDS, + ) + if not response.ok: + raise RuntimeError( + f"Slack returned HTTP {response.status_code}: {response.text.strip()}" + ) + + # chat.postMessage reports application errors as HTTP 200 with ok=false, so the + # body is what has to be checked rather than the status. + result = response.json() + if not result.get("ok"): + reason = result.get("error", result) + # On missing_scope Slack names the scope it wanted and the ones the token + # carries. Without those two the error is very hard to act on. + if result.get("needed"): + reason += f" (needed {result['needed']}, token has {result['provided']})" + raise RuntimeError(f"error posting slack message: {reason}") + + return result["ts"] diff --git a/tests/test_slack.py b/tests/test_slack.py new file mode 100644 index 000000000..aa7605754 --- /dev/null +++ b/tests/test_slack.py @@ -0,0 +1,131 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +import json + +import pytest +import responses + +from bugbot import slack + +CHANNEL = "C0123456789" +TEST_API_URL = "https://slack.test/api/" +POST_MESSAGE_URL = f"{TEST_API_URL}chat.postMessage" + + +@pytest.fixture +def api(monkeypatch): + """Point the transport at a test server and give it a token to use.""" + monkeypatch.setenv(slack.API_URL_VAR, TEST_API_URL) + monkeypatch.setenv(slack.TOKEN_VAR, "xoxb-test") + + +@responses.activate +def test_posts_the_channel_text_and_token(api): + responses.add(responses.POST, POST_MESSAGE_URL, json={"ok": True, "ts": "1.2"}) + + assert slack.post_to_slack(CHANNEL, "a title") == "1.2" + + request = responses.calls[0].request + assert request.headers["Authorization"] == "Bearer xoxb-test" + payload = json.loads(request.body) + assert payload["channel"] == CHANNEL + assert payload["text"] == "a title" + # Unfurls would repeat what a link-heavy message already says. + assert payload["unfurl_links"] is False + assert payload["unfurl_media"] is False + # Posted under a readable name rather than the Slack app's own. + assert payload["username"] == slack.USERNAME + # Neither was passed, so neither should be sent. + assert "blocks" not in payload + assert "thread_ts" not in payload + + +@responses.activate +def test_posts_blocks_and_replies_in_thread(api): + responses.add(responses.POST, POST_MESSAGE_URL, json={"ok": True, "ts": "3.4"}) + + slack.post_to_slack( + CHANNEL, + "a title", + blocks=[{"type": "divider"}], + thread_ts="1.2", + ) + + payload = json.loads(responses.calls[0].request.body) + assert payload["blocks"] == [{"type": "divider"}] + assert payload["thread_ts"] == "1.2" + + +@responses.activate +def test_an_application_error_comes_back_as_http_200(api): + # Slack reports a rejected payload with ok=false and a 200, so the status + # alone would read as success. + responses.add( + responses.POST, POST_MESSAGE_URL, json={"ok": False, "error": "invalid_blocks"} + ) + + with pytest.raises(RuntimeError, match="invalid_blocks"): + slack.post_to_slack(CHANNEL, "a title") + + +@responses.activate +def test_missing_scope_names_the_scope_it_wanted(api): + responses.add( + responses.POST, + POST_MESSAGE_URL, + json={ + "ok": False, + "error": "missing_scope", + "needed": "chat:write", + "provided": "im:read", + }, + ) + + with pytest.raises(RuntimeError, match=r"missing_scope \(needed chat:write"): + slack.post_to_slack(CHANNEL, "a title") + + +@responses.activate +def test_an_http_error_reports_the_body(api): + responses.add(responses.POST, POST_MESSAGE_URL, body="nope", status=500) + + with pytest.raises(RuntimeError, match="HTTP 500: nope"): + slack.post_to_slack(CHANNEL, "a title") + + +def test_the_environment_token_wins_over_the_configured_one(monkeypatch): + monkeypatch.setenv(slack.TOKEN_VAR, "xoxb-env") + monkeypatch.setattr( + slack.utils, "get_login_info", lambda: {"slack_bot_token": "xoxb-cfg"} + ) + + assert slack.get_token() == "xoxb-env" + + monkeypatch.delenv(slack.TOKEN_VAR) + assert slack.get_token() == "xoxb-cfg" + + +def test_a_missing_token_stops_the_run(monkeypatch): + monkeypatch.delenv(slack.TOKEN_VAR, raising=False) + monkeypatch.setattr(slack.utils, "get_login_info", lambda: {}) + + with pytest.raises(RuntimeError, match="chat:write"): + slack.get_token() + + +def test_a_missing_config_file_is_a_missing_token_not_a_crash(monkeypatch): + def no_file(): + raise FileNotFoundError("configs/config.json") + + monkeypatch.delenv(slack.TOKEN_VAR, raising=False) + monkeypatch.setattr(slack.utils, "get_login_info", no_file) + + with pytest.raises(RuntimeError, match="chat:write"): + slack.get_token() + + +def test_the_username_is_spelled_correctly(): + # It is on every message anyone reads, so a typo here is very visible. + assert slack.USERNAME == "Firefox Release Management Bot" From 4e1c19641ad1ef9edb68faf8d7c050b1598af25e Mon Sep 17 00:00:00 2001 From: DonalMe <76789979+DonalMe@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:56:21 -0400 Subject: [PATCH 2/6] Post the REO release regression status to Slack Ported from mozilla/releases_insights, where this ran as a GitHub Action over anonymous Bugzilla queries. That is the reason for the move: an anonymous search never returns a restricted bug, so every count it posted silently excluded them. Running as bugbot, the same queries see them. Two rules under bugbot/rules, one per message, over the queries and formatting they share in bugbot/reo_regressions.py: - the cycle summary, `reo_regression_slack`, posted Mon and Thu, splitting each channel's open regressions into new and carry over - the action required message, `reo_regression_slack_daily`, ungated and so posted every weekday, listing only the regressions stuck long enough to need a nudge, plus the fixes nobody has asked to uplift The summary decides its two days in the rule rather than in configs/rules.json, as `missed_uplifts` and `workflow.p2_merge_day` decide theirs: the cadence is part of what that message is -- it reports how a cycle is going rather than what has just changed, and says so in its own wording -- so a day added to it in configuration would not make the message any more true. Restricted bugs are counted and linked like any other but never named: no message prints a bug summary, which is the line `BzCleaner.get_summary` draws. The top-level bullet says how many of its count are restricted, because a reader without access opens the link and finds a shorter list than the number they clicked on. One query change worth calling out: `burndown_query` narrows "still marked affected" to the fixes worth chasing, and being in a security group is one of the three ways in. Under anonymous queries that branch matched nothing, so the burndown lines grow here beyond the general restricted-bug increase. Neither rule is a BzCleaner. They run several queries rather than one, report counts rather than a table of bugs, write nothing to Bugzilla, and have to post on a quiet day to say so -- where BzCleaner sends nothing when there are no bugs, and its cache would suppress a bug that must reappear until fixed. They borrow the conventions that do apply: a module per rule in bugbot/rules with the shared parts in a top-level module the way `multinaggers` and `topcrash` hold theirs, a base class rules subclass included, `description`, `must_run(date)` and a name taken from the module file all meaning what they mean there, --production, `logger_extra["bugbot_rule"]`, and the shared helpers for versions, flag names, component teams and search URLs. Testing and Developer Infrastructure are dropped from every query, so their bugs reach neither message and no bucket in either. One ANDed notequals per product rather than a single nowords: Bugzilla splits a nowords value on whitespace, so "Developer Infrastructure" would match as two separate words and drop products nobody asked to exclude. Both rules run from the weekday cron script and post to `CHANNEL`, which is in the code rather than configs/rules.json: it is not a secret, and changing where an unattended recurring message lands should take a code review. CHANNEL is #tmp-dm-test for now, a scratch channel to shake the port out in alongside the messages it replaces. It has to point at the real REO channel before either message is meant for anyone to read. --- README.rst | 2 +- bugbot/reo_regressions.py | 612 ++++++++++++++++++ bugbot/rules/reo_regression_slack.py | 266 ++++++++ bugbot/rules/reo_regression_slack_daily.py | 401 ++++++++++++ scripts/cron_run_weekdays.sh | 7 + tests/rules/test_reo_regression_slack.py | 69 ++ .../rules/test_reo_regression_slack_daily.py | 145 +++++ tests/test_reo_regressions.py | 241 +++++++ 8 files changed, 1742 insertions(+), 1 deletion(-) create mode 100644 bugbot/reo_regressions.py create mode 100644 bugbot/rules/reo_regression_slack.py create mode 100644 bugbot/rules/reo_regression_slack_daily.py create mode 100644 tests/rules/test_reo_regression_slack.py create mode 100644 tests/rules/test_reo_regression_slack_daily.py create mode 100644 tests/test_reo_regressions.py diff --git a/README.rst b/README.rst index 6c6545c0e..df1f4b0e8 100644 --- a/README.rst +++ b/README.rst @@ -65,7 +65,7 @@ Before running: The ``hackbot_api_key`` is only needed by rules that start a `hackbot `_ agent run (currently ``frontend_triage``). Those rules talk to ``https://hackbot-api.moz.tools`` by default; set ``HACKBOT_API_URL`` to point at a different deployment. -The ``slack_bot_token`` is only needed by rules that post a message to Slack. It is a Slack app's Bot User OAuth Token and needs the ``chat:write``, ``chat:write.customize`` and ``chat:write.public`` scopes. +The ``slack_bot_token`` is only needed by rules that post a message to Slack (currently ``reo_regression_slack`` and ``reo_regression_slack_daily``). It is a Slack app's Bot User OAuth Token and needs the ``chat:write``, ``chat:write.customize`` and ``chat:write.public`` scopes. Do a dryrun:: diff --git a/bugbot/reo_regressions.py b/bugbot/reo_regressions.py new file mode 100644 index 000000000..332eedc16 --- /dev/null +++ b/bugbot/reo_regressions.py @@ -0,0 +1,612 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +"""Shared parts of the rules that post the REO release regressions to Slack. + +Two rules are built on this, both over the bug set behind the REO tab of +https://bugdash.moz.tools/: + +- `bugbot.rules.reo_regression_slack`, the cycle summary, Mon and Thu +- `bugbot.rules.reo_regression_slack_daily`, the action required message, every + weekday + +What lives here is mostly what both of them need: the open regressions query, +the Bugzilla search and link building, the team breakdown, the Block Kit +wrapping and the `ReoRegressionsRule` base class that runs one of them. What is +one message's own -- its heading, its cadence, its buckets -- lives in that +rule. + +A few things here have one caller today, and each says so where it is defined. +They are kept here because of what they are rather than who uses them: a +condition on a query this module builds, or a fact about Slack's markup. Moving +each one to whichever rule happens to call it would leave the next reader +looking in two files for one vocabulary, and moving it back is what adding the +second caller would mean. + +Shaped after `bugbot.multinaggers` and `bugbot.topcrash`: a module here holding +what rules under `bugbot/rules` share, base class included. + +Restricted bugs are counted in the totals and included in the links like any +other, but never named: no message prints a bug summary, which is the same line +`BzCleaner.get_summary` draws. The top-level bullet says how many of its count +are restricted, because a reader without access opens the link and finds a +shorter list than the number they clicked on, and the note is what explains the +gap. See `restricted_note`. +""" + +import argparse +import datetime +import functools +import os +import sys +from collections.abc import Collection + +from libmozdata.bugzilla import Bugzilla + +from bugbot import logger, logger_extra, slack, utils +from bugbot.components import ComponentName, fetch_component_teams + +# Shared with the rest of bugbot rather than redeclared as ("S1", "S2") the way the +# REO queries have it. It also carries the pre-S1 names — critical maps to S1, major +# and blocker to S2 (see `constants.OLD_SEVERITY_MAP`) — so an old bug that never had +# its severity restated still lands in the S2+ counts instead of quietly missing from +# them. That is a wider net than bugdash casts, so these numbers can run slightly +# ahead of the REO tab's. +# +# Re-exported here rather than imported by each rule, so that reasoning is written +# down once for both messages. +from bugbot.constants import HIGH_SEVERITY # noqa: F401 + +# The channel both rules post to. Here rather than in `configs/rules.json` +# because it is not a secret, and because changing where an unattended recurring +# message lands should take a code review -- the same reasoning `frontend_triage` +# gives for keeping its component list in code. The bot token is the part that is +# a secret, and that stays in `configs/config.json`. +# +# TEMPORARY: this is currently #tmp-dm-test, a scratch channel for shaking the +# port out. It has to be pointed at the real REO channel before either message is +# meant for anyone to read. +CHANNEL = "C0BLP0WUBED" + +BZ_BUGLIST_URL = "https://bugzilla.mozilla.org/buglist.cgi" + +# Every Bugzilla classification except Graveyard, which holds the ~100 retired +# products. Same list bugdash's REO queries use. +# +# "Developer Infrastructure" appearing here and in EXCLUDED_PRODUCTS is not a +# contradiction: the classification also holds Firefox Build System, Conduit and +# Tree Management, which stay in scope, and only the product of the same name is +# dropped. +CLASSIFICATIONS = [ + "Client Software", + "Components", + "Developer Infrastructure", + "Other", + "Server Software", +] + +# The severity that means no triage decision has been made yet. Bugs are filtered +# on this locally, so the value has to be exactly what Bugzilla reports in a bug's +# severity field, which is case sensitive and not always what the same value looks +# like in a search: "N/A" comes back from the API where a query matches it as +# "n/a". Only "--" counts as missing here; N/A is a decision, not the absence of +# one. +MISSING_SEVERITIES = ("--",) + +# Products dropped from every query, so their bugs reach neither message and no +# bucket in them. Excluded at the query rather than per bucket, so a product here +# is out of the cycle summary, all the daily buckets and the burndown lines alike. +# +# Not the `exclude_products` key some rules carry in `configs/rules.json`: that one +# subtracts from `BzCleaner`'s default product list, which these classification +# scoped queries never use, so the name would mean something different here. +EXCLUDED_PRODUCTS = ("Testing", "Developer Infrastructure") + +# Where the product exclusions are numbered from in a boolean chart. Above every +# slot either query uses -- `regressions_query` here and `burndown_query` in the +# daily rule -- including the 11 `with_severities` takes. +EXCLUDED_PRODUCTS_SLOT = 12 + +# For a component with no team_name, or one missing from the mapping entirely. +# Every component had a team when this was written, so this is only a guard +# against silently dropping bugs out of the per-team line. +UNKNOWN_TEAM = "Unknown team" + +# A Slack section block holds at most 3000 characters. +SECTION_LIMIT = 3000 + +# Above this length a snapshot URL is shortened, and failing that swapped for the +# query URL or dropped entirely -- see `bug_link`. Keeps one very long bug list +# from pushing a section over SECTION_LIMIT. +MAX_SNAPSHOT_URL = 2000 + +# Slack renders this back as >. Sending the character itself would work where it +# is used now, but it ends a link's label at the first > and opens a blockquote at +# the start of a line, so a label or bullet reworded around it would break in ways +# that are easy to miss. The entity is never wrong. +# +# The daily rule's "> 24 hours" is the only use today. It is here rather than +# there because it is a fact about Slack's markup, like SUB_BULLET below, and not +# about that message. +GREATER_THAN = ">" + +# Slack has no nested lists in message text, so indent sub-bullets by hand. +# Four plain spaces; if Slack ever collapses them, non-breaking spaces (U+00A0) +# are the fix. +SUB_BULLET = " ◦ " + +# What a bug search has to come back with for either message. The cycle summary +# needs no more than this and takes it as `fetch_bugs`'s default; the daily rule +# extends it, as it ages every bug and the timestamps it ages from live on the +# bug itself. +# +# `groups` is in here rather than in one of those extensions: it is how a bug is +# known to be restricted, and every message counts those. See `restricted_note`. +# +# No `summary` field, here or in either extension. That is the line neither +# message crosses, and the same one `BzCleaner.get_summary` draws. +BUG_FIELDS = "id,severity,product,component,groups" + + +def utc_today() -> datetime.date: + """Today in UTC: milestone dates are UTC and the cron host may not be.""" + return datetime.datetime.now(datetime.timezone.utc).date() + + +def without_excluded_products(slot: int = EXCLUDED_PRODUCTS_SLOT) -> dict: + """Chart conditions dropping EXCLUDED_PRODUCTS, a numbered slot per product. + + One ANDed notequals per product rather than a single nowords: Bugzilla splits + a nowords value on whitespace, so "Developer Infrastructure" would be matched + as the two words separately and drop products nobody asked to exclude. + """ + conditions: dict = {} + for offset, product in enumerate(EXCLUDED_PRODUCTS): + number = slot + offset + conditions |= { + f"f{number}": "product", + f"o{number}": "notequals", + f"v{number}": product, + } + + return conditions + + +def status_flag(version: int) -> str: + """The status flag for a Firefox version, e.g. `cf_status_firefox150`. + + Built by `utils.get_flag` rather than concatenated: that is the one place + version numbers become flag names anywhere in bugbot. Its channel argument only + changes the name for ESR, and every version these queries run over is a desktop + one, so which channel the version happens to be on doesn't enter into it. + """ + return utils.get_flag(version, "status", "release") + + +def tracking_flag(version: int) -> str: + """The tracking flag for a Firefox version, e.g. `cf_tracking_firefox150`. + + See `status_flag` for why this goes through `utils.get_flag`. + """ + return utils.get_flag(version, "tracking", "release") + + +def regressions_query(version: int, carry_over: bool | None = None) -> dict: + """Build the open regressions query for a version. + + Bugs with all of the following: + - regression keyword + - open (unresolved) + - status-firefox{version} is affected + Bugs with any of the following are ignored: + - tracking-firefox{version} is - + - stalled or intermittent-failure keywords + - within one of EXCLUDED_PRODUCTS + + carry_over adds a condition on the previous version, splitting that set in + two. False keeps the bugs where status-firefox{version - 1} is one of + unaffected, ? or ---, so they regressed during this cycle; True negates it, + leaving the ones that were already there. The two therefore partition every + open regression affecting the version, and the default of None asks for that + whole set instead of one side of it. + + Nothing here filters on `bug_group`: an authenticated search returns every bug + the key can see, so restricted regressions arrive on their own. + + Field numbering is Bugzilla's boolean charts: f/o/v are the field, operator + and value for a numbered condition, OP and CP open and close a group, j sets + how a group joins (OR here, AND otherwise) and n negates. The gaps at f7 and + f9 are harmless, as Bugzilla ignores unused numbers: f7 comes from bugdash, + f9 from the product exclusions moving to EXCLUDED_PRODUCTS_SLOT. + """ + query = { + "classification": CLASSIFICATIONS, + "keywords": "regression", + "keywords_type": "allwords", + "resolution": "---", + "f1": status_flag(version), + "o1": "equals", + "v1": "affected", + "f8": tracking_flag(version), + "o8": "notequals", + "v8": "-", + "f10": "keywords", + "o10": "nowordssubstr", + "v10": "stalled,intermittent-failure", + **without_excluded_products(), + } + + if carry_over is None: + return query + + # Conditions are matched up by their number, so leaving these out above and + # adding them here changes nothing but the order they appear in the URL. + previous = status_flag(version - 1) + query |= { + "f2": "OP", + "j2": "OR", + "f3": previous, + "o3": "equals", + "v3": "unaffected", + "f4": previous, + "o4": "equals", + "v4": "?", + "f5": previous, + "o5": "equals", + "v5": "---", + "f6": "CP", + } + + if carry_over: + # n2 attaches to the OP at f2, so it negates the whole f3-f5 group rather + # than just the first condition in it. + query["n2"] = "1" + + return query + + +def with_severities(query: dict, severities: Collection[str]) -> dict: + """Narrow a query to some severities, for a link that stays live. + + The counts themselves are filtered locally, so this is only needed to build a + URL when a bug list is too long to link by id. Slot 11 is free: the regressions + query leaves it unused, and EXCLUDED_PRODUCTS_SLOT starts above it. + + The cycle summary is the only caller today. It is here rather than there + because it edits `regressions_query`'s chart, and which slot it may take can + only be answered next to the slots that query and the daily rule's burndown + query have already spoken for. + + Sorted so the same set of severities always produces the same URL: the order a + set iterates in is not stable from one process to the next, and `HIGH_SEVERITY` + is a set. + """ + return { + **query, + "f11": "bug_severity", + "o11": "anyexact", + "v11": ", ".join(sorted(severities)), + } + + +@functools.cache +def component_teams() -> dict[ComponentName, str]: + """Map every (product, component) to the team that owns it. + + team_name is a Bugzilla field on components, the same one bugdash's Teams + filter uses. One request covers every product, around 120KB for 2000-odd + components, which is why it's cached for the life of the run. + """ + return fetch_component_teams() + + +def team_of(bug: dict) -> str: + """The team owning a bug's component.""" + return component_teams().get(ComponentName.from_bug(bug)) or UNKNOWN_TEAM + + +def fetch_bugs(query: dict, fields: str = BUG_FIELDS) -> list[dict]: + """Return the requested fields of every bug matching a query. + + Fetching the bugs rather than asking for count_only is what lets the severity + and team breakdowns be derived from one request, and lets each count link to + the exact bugs behind it. + + `fields` defaults to the shared list, which is everything the cycle summary + asks of a bug; the daily rule passes its own, longer one. + + libmozdata pages the search itself — counting first, then walking the results in + chunks — but only for a query carrying none of count_only, limit, order or + offset, so none of those may be added here. It also attaches bugbot's API key, + which is the whole reason these rules see restricted bugs at all. + """ + bugs: list[dict] = [] + + Bugzilla( + {**query, "include_fields": fields}, + bughandler=lambda bug, data: data.append(bug), + bugdata=bugs, + timeout=utils.get_config("common", "bz_query_timeout"), + ).get_data().wait() + + return bugs + + +def query_url(query: dict) -> str: + """A Bugzilla URL that re-runs a query, so its results change over time.""" + return utils.get_bz_search_url(query) + + +def snapshot_url(bugs: list[dict]) -> str: + """A Bugzilla URL listing exactly these bugs, as bugdash's bug lists do. + + Linking the bug ids rather than the query means the list still matches the + count in the message when it is read days later. order=bug_list keeps + Bugzilla showing them in the order given rather than re-sorting. + + Built by hand rather than through `utils.get_bz_search_url` so the separators + stay as commas: percent-encoded they would triple in length, and the length is + what `MAX_SNAPSHOT_URL` is measuring. + + Restricted bugs are in here with everything else. A reader without access gets + a shorter list than the count that linked them here, which is what the + "(n restricted)" note on the bullet is for. + """ + ids = ",".join(str(bug["id"]) for bug in bugs) + + return f"{BZ_BUGLIST_URL}?bug_id={ids}&order=bug_list" + + +def shortened_url(url: str) -> str | None: + """A short Bugzilla URL for a long one, or None if it couldn't be shortened. + + `utils.shorten_long_bz_url` answers a shortener error by returning the URL + split across several lines (bugbot#1402). Harmless in an email, useless in a + Slack link, which would end at the first newline — so a multi-line answer is + treated as a failure here rather than posted. + + Any other failure is swallowed for the same reason: the count is the message + and the link is a convenience, so a shortener that is down should cost the link + and nothing more. + """ + try: + short = utils.shorten_long_bz_url(url) + except Exception: + logger.exception("Could not shorten a Bugzilla URL") + return None + + if "\n" in short or len(short) > MAX_SNAPSHOT_URL: + return None + + return short + + +def bug_link( + bugs: list[dict], label_template: str, fallback_query: dict | None = None +) -> str: + """Format a non-empty bug list as a Slack link labelled with its count. + + label_template is formatted with the count, e.g. "{} New Regressions". + + A snapshot URL that comes out too long is shortened, which keeps the link + pointing at exactly the bugs counted. Failing that it falls back to + fallback_query, which is a live query and so can drift from the count beside + it, and failing that the count is left unlinked. Team lines pass no fallback, + as reproducing a team as a query means listing all its components, so before + the shortener they lost their link entirely. + + Callers are expected to skip empty lists: an empty bug_id would link to a + broken list, and a count of zero is left out of the message anyway. + """ + label = label_template.format(len(bugs)) + snapshot = snapshot_url(bugs) + + if len(snapshot) <= MAX_SNAPSHOT_URL: + return f"<{snapshot}|{label}>" + + url = shortened_url(snapshot) + if url is None and fallback_query is not None: + url = query_url(fallback_query) + + if url is None: + return label + + return f"<{url}|{label}>" + + +def restricted_note(bugs: list[dict]) -> str: + """Say how many of a bug list are restricted, or nothing when none are. + + A bug is restricted when it is in any group at all, not only a security one: + the note exists to explain why the linked list looks shorter than the count to + a reader without access, and that gap opens for an employee-confidential or + partner group just as it does for `core-security`. That is a wider test than + the `bug_group ~ "sec"` branch in the daily rule's burndown query, which is + asking a different question — whether a fix is worth chasing, not whether it is + readable. + + Deliberately plain text rather than part of the link label, so the blue runs as + far as the thing being counted and no further, and deliberately only used on the + top-level bullets: repeated on every severity and team sub-bullet it would say + little and crowd out the counts that are the point of those lines. + """ + count = sum(1 for bug in bugs if bug.get("groups")) + if not count: + return "" + + return f" ({count} restricted)" + + +def team_breakdown(bugs: list[dict]) -> str: + """Count the bugs owned by each team, busiest team first. + + Every team is listed rather than just the top few, so that the line works + as a nudge to each team that owns something. + """ + by_team: dict[str, list[dict]] = {} + for bug in bugs: + by_team.setdefault(team_of(bug), []).append(bug) + + ranked = sorted(by_team.items(), key=lambda item: (-len(item[1]), item[0])) + + return ", ".join(bug_link(team_bugs, f"{{}} {team}") for team, team_bugs in ranked) + + +def to_blocks(sections: list[str]) -> list[dict]: + """Wrap the sections of a message as Block Kit sections. + + Slack silently splits a message whose text runs past about 4000 characters + into several messages, which is what happened when every count linked to a + full query URL. Snapshot URLs brought the total well under that, but each + section block gets its own 3000 character allowance, so keeping the sections + means a busier cycle can't start splitting the message again. + + A section that does overflow raises rather than posting something malformed. + The team breakdown is the part that could get there, at roughly 90 characters + per team; capping or splitting it is the fix if that ever fires. + """ + for section in sections: + if len(section) > SECTION_LIMIT: + raise RuntimeError( + f"Slack section block is {len(section)} characters, over the " + f"{SECTION_LIMIT} limit:\n{section[:200]}..." + ) + + return [ + {"type": "section", "text": {"type": "mrkdwn", "text": section}} + for section in sections + ] + + +def block_text(block: dict) -> str: + """The text of any block, for printing a message instead of posting it. + + Section and header blocks keep their text in one place and context blocks in + a list of elements, so a dry run has to handle both rather than assume the + shape of the blocks it was handed. + """ + if "elements" in block: + return " ".join(element["text"] for element in block["elements"]) + + return block["text"]["text"] + + +class ReoRegressionsRule: + """Base for the rules that post a REO release regression message to Slack. + + A subclass says what its message is called, when it runs and what is in it; + building and posting it is the same either way and happens here. + + Not a `BzCleaner`: these rules run several queries rather than one, report + counts rather than a table of bugs, write nothing to Bugzilla, and have to + post on a quiet day to say so -- where `BzCleaner` sends nothing when there + are no bugs, and its cache would suppress a bug that must reappear until it + is fixed. The conventions that do apply are borrowed rather than reinvented: + `description`, `must_run(date)` and a `name` taken from the module file all + mean what they mean there, `--production` is the flag that makes a run real, + `logger_extra["bugbot_rule"]` tags the log, and a failure is logged and + swallowed so one message can't fail the whole cron job. + """ + + def __init__(self) -> None: + self.__rule_name__ = self._rule_name() + + def _rule_name(self) -> str: + """The rule name, taken from the module file as `BzCleaner` takes it.""" + module = sys.modules[self.__class__.__module__] + module_file = module.__file__ + assert module_file is not None + + return os.path.splitext(os.path.basename(module_file))[0] + + def name(self) -> str: + """Get the rule name""" + return self.__rule_name__ + + def description(self) -> str: + """Get the description for the help""" + return "" + + def heading(self) -> str: + """The message's title, which is also its notification fallback text.""" + raise NotImplementedError + + def must_run(self, date: datetime.date) -> bool: + """Check if the rule must run for this date""" + return True + + def blocks(self, versions: dict[str, int]) -> list[dict]: + """Build the message as Block Kit blocks.""" + raise NotImplementedError + + def get_args_parser(self) -> argparse.ArgumentParser: + """Get the arguments from the command line""" + parser = argparse.ArgumentParser(description=self.description()) + parser.add_argument( + "--production", + dest="dryrun", + action="store_false", + help=( + "If the flag is not passed, just build the message and print it to " + "the console without posting it to Slack" + ), + ) + parser.add_argument( + "--channel", + action="store", + default="", + help=( + f"Slack channel ID to post to, overriding {CHANNEL}. Useful to shake " + "the message out somewhere else without editing the code." + ), + ) + parser.add_argument( + "--force", + action="store_true", + help=( + "Post even on a day the rule's own must_run would skip. No effect " + "on a rule that posts whenever it is invoked" + ), + ) + + return parser + + def run(self) -> None: + """Run the rule""" + logger_extra["bugbot_rule"] = self.name() + logger.info("Run rule %s", self.name()) + + args = self.get_args_parser().parse_args() + + try: + if not args.force and not self.must_run(utc_today()): + logger.info( + "%s: not a day this message runs on; --force overrides", + self.name(), + ) + return + + versions = utils.get_versions_from_trains() + blocks = self.blocks(versions) + + if args.dryrun: + print("DRY RUN: message not posted.\n") + for block in blocks: + print(block_text(block)) + return + + slack.post_to_slack(args.channel or CHANNEL, self.heading(), blocks=blocks) + logger.info( + "Rule %s posted for Firefox %s / %s / %s", + self.name(), + versions["release"], + versions["beta"], + versions["nightly"], + ) + except Exception: + # Logged and swallowed, as `BzCleaner.run` does: `bugbot.log --send` + # mails the digest at the end of the cron run, so the failure is seen + # without a non-zero exit tripping the ERR trap and failing the whole + # job over one message. + logger.exception("Rule %s", self.name()) diff --git a/bugbot/rules/reo_regression_slack.py b/bugbot/rules/reo_regression_slack.py new file mode 100644 index 000000000..4a2ba3851 --- /dev/null +++ b/bugbot/rules/reo_regression_slack.py @@ -0,0 +1,266 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +"""Post the REO release regression cycle summary to Slack. + +For Release, Beta and Nightly it reports two bug lists, built from the bug set +behind the REO tab of https://bugdash.moz.tools/: + +- "new regressions" carry the regression keyword and are affected in version N + while N-1 is unaffected or unknown, so they regressed during this cycle +- "carry over regressions" are the same query negated: N-1 has a real status, so + the bug was already there + +Those two partition every open regression affecting N. Each count is broken down +by severity, with New Regressions also broken down by owning team. Beta and +Nightly get a working day countdown to the end of their cycle. + +Every count links to a Bugzilla list of exactly the bugs counted. Restricted +bugs are counted and linked like any other but never named; see +`bugbot.reo_regressions`, which also holds the queries and the posting. + +The regressions that need chasing rather than counting are the other rule, +`reo_regression_slack_daily`. +""" + +import datetime +import functools +import re + +import requests +from libmozdata import utils as lmdutils +from libmozdata.fx_trains import FirefoxTrains + +from bugbot import reo_regressions as reo +from bugbot import utils + +# The days this message is posted on. In the rule rather than in +# `configs/rules.json` because the cadence is part of what the message is: it +# reports how a cycle is going rather than what has just changed, and it says so +# in its own wording, so a day added to it in configuration would not make the +# message it produces any more true. `missed_uplifts` and `workflow.p2_merge_day` +# decide their days in the rule for the same reason. +# +# Twice a week rather than daily: the counts move slowly, and a summary that +# arrives every morning stops being read. +MUST_RUN_DAYS = ("Mon", "Thu") + +RELEASE_PAGE_URL = "https://whattrainisitnow.com/release/?version={}" + +WELLNESS_API_URL = "https://whattrainisitnow.com/api/wellness/days/" + +# The wellness endpoint answers quickly; the Bugzilla searches get bugbot's own +# `bz_query_timeout`, which is far longer. +HTTP_TIMEOUT_SECONDS = 15 + +# Stands in for a milestone key, as the last beta is numbered differently from +# one version to the next (beta_10 for 154, beta_5 under the 2 week cadence). +LAST_BETA = "last_beta" + +# The milestone that ends each channel's cycle, and the cycle's name. Both the +# countdown ("End of Beta ...") and the finished line ("Beta cycle finished") are +# built from that one name, so they can't drift apart. Release has no equivalent +# deadline, so it gets no countdown. +CYCLE_ENDS = { + "beta": ("Beta", LAST_BETA), + "nightly": ("Nightly", "merge_day"), +} + +# Custom emoji in the Mozilla workspace, one per channel. A name that doesn't exist +# there renders as the literal :name: rather than failing, so these have to stay +# in step with the workspace. +CHANNEL_EMOJI = { + "release": ":firefox-browser:", + "beta": ":beta-browser:", + "nightly": ":nightly-browser:", +} + +HEADING = "REO release regression status:" + +# Shown instead of dropping a channel entirely, so a silent channel reads as +# good news rather than as the script having failed. +NOTHING_TO_REPORT = "• No open release regressions" + + +@functools.cache +def wellness_days() -> frozenset[datetime.date]: + """Fetch the days off that don't count as working days. + + libmozdata's `FirefoxTrains` covers the schedule and owners endpoints but not + this one, so it is fetched directly. Moving it there is the tidier home if a + second caller ever turns up. + """ + response = requests.get( + WELLNESS_API_URL, + headers={"User-Agent": "bugbot"}, + timeout=HTTP_TIMEOUT_SECONDS, + ) + response.raise_for_status() + + return frozenset(datetime.date.fromisoformat(day) for day in response.json()) + + +def work_days_until(end: datetime.date) -> int: + """Count working days between today and end, end excluded. + + Mirrors ReleaseInsights\\Duration::workDays() so this agrees with the + countdowns on the release pages: weekends, wellness days and the current + day are all left out. + """ + today = reo.utc_today() + days = (end - today).days + if days <= 0: + return 0 + + # Counting from tomorrow is what leaves the current day out. + return sum( + 1 + for offset in range(1, days) + if (day := today + datetime.timedelta(days=offset)).weekday() < 5 # Mon-Fri + and day not in wellness_days() + ) + + +def release_schedule(version: int) -> dict: + """A version's milestone dates from the trains API, cached by libmozdata.""" + return FirefoxTrains.get_instance().get_release_schedule(str(version)) + + +def milestone_date(schedule: dict, milestone: str) -> datetime.date: + """The date of a milestone, resolving LAST_BETA to the highest numbered beta. + + The number of betas differs per version, so the last one has to be found + rather than named. Sorting on the number matters: as strings, beta_9 would + come after beta_10. + """ + if milestone == LAST_BETA: + betas = [key for key in schedule if re.fullmatch(r"beta_\d+", key)] + milestone = max(betas, key=lambda key: int(key.removeprefix("beta_"))) + + return lmdutils.get_date_ymd(schedule[milestone]).date() + + +def cycle_countdown(version: int, channel: str) -> str: + """A countdown to the end of this version's time on the channel. + + Beta ends with the last beta build; Nightly ends on merge day, when the + version moves to Beta. Release has no such deadline. + + The version numbers roll over on merge day, so the day of and the days after + that deadline each only show up briefly, but they read badly as a countdown + ("in 0 working days") and so get their own wording. + """ + if channel not in CYCLE_ENDS: + return "" + + cycle, milestone = CYCLE_ENDS[channel] + label = f"End of {cycle}" + end = milestone_date(release_schedule(version), milestone) + today = reo.utc_today() + + if end < today: + return f"{cycle} cycle finished" + + if end == today: + return f"{label} today" + + if end == today + datetime.timedelta(days=1): + return f"{label} {end:%Y-%m-%d} — tomorrow" + + days = work_days_until(end) + return f'{label} {end:%Y-%m-%d} in {days} {utils.plural("working day", days)}' + + +def regression_group( + version: int, carry_over: bool, label: str, by_team: bool = False +) -> str: + """Build the bullet and severity sub-bullets for one bug list. + + The list is fetched once and split by severity and team here, rather than + asking Bugzilla for each subset, so the sub-bullets are guaranteed to be + part of the count above them. + + Bug lists that are empty are left out entirely rather than reported as a + zero, so a quiet channel is short instead of a wall of "0". Returns an + empty string when there are no bugs at all. + """ + query = reo.regressions_query(version, carry_over) + bugs = reo.fetch_bugs(query) + if not bugs: + return "" + + link = reo.bug_link(bugs, f"{{}} {label} Regressions", query) + lines = [f"• {link}{reo.restricted_note(bugs)}"] + + if by_team: + lines.append(reo.SUB_BULLET + reo.team_breakdown(bugs)) + + severity_counts = [] + for severities, template in ( + (reo.HIGH_SEVERITY, "{} S2+"), + (reo.MISSING_SEVERITIES, "{} missing severity"), + ): + subset = [bug for bug in bugs if bug["severity"] in severities] + if subset: + severity_counts.append( + reo.bug_link(subset, template, reo.with_severities(query, severities)) + ) + + if severity_counts: + lines.append(reo.SUB_BULLET + ", ".join(severity_counts)) + + return "\n".join(lines) + + +class ReoRegressionSlack(reo.ReoRegressionsRule): + """Post the state of this cycle's open release regressions to Slack.""" + + def description(self) -> str: + return "REO release regression cycle summary posted to Slack" + + def heading(self) -> str: + return HEADING + + def must_run(self, date: datetime.date) -> bool: + weekdays = utils.get_weekdays() + + return any(weekdays[day] == date.weekday() for day in MUST_RUN_DAYS) + + def blocks(self, versions: dict[str, int]) -> list[dict]: + """Build the cycle summary as Block Kit sections, one per bug list.""" + sections = [HEADING] + + for channel in ("release", "beta", "nightly"): + version = versions[channel] + page = RELEASE_PAGE_URL.format(version) + emoji = CHANNEL_EMOJI[channel] + header = f"{emoji} *<{page}|Fx{version} {channel.title()}>*" + + countdown = cycle_countdown(version, channel) + if countdown: + header += f"\n{countdown}" + + groups = [ + group + for group in ( + regression_group(version, False, "New", by_team=True), + regression_group(version, True, "Carry Over"), + ) + if group + ] + + if not groups: + sections.append(f"{header}\n{NOTHING_TO_REPORT}") + continue + + # The header rides along with the first surviving group, so that a + # channel with only carry over bugs isn't left with a stray heading. + sections.append(f"{header}\n{groups[0]}") + sections.extend(groups[1:]) + + return reo.to_blocks(sections) + + +if __name__ == "__main__": + ReoRegressionSlack().run() diff --git a/bugbot/rules/reo_regression_slack_daily.py b/bugbot/rules/reo_regression_slack_daily.py new file mode 100644 index 000000000..ac9a60604 --- /dev/null +++ b/bugbot/rules/reo_regression_slack_daily.py @@ -0,0 +1,401 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +"""Post the REO release regressions that need action to Slack. + +The same bug set as the cycle summary, `reo_regression_slack`, with the new and +carry over split dropped and the three channels merged into one deduplicated +list, reporting only the bugs stuck long enough to need a nudge: high severity +with nobody on them (UNASSIGNED_EXEMPT_* exempt), no severity decision, or an +unanswered needinfo. It ends with bugdash's Burndown list per version, Beta and +Release only, cut down to the fixes nobody has asked to uplift. Each line is +broken down by owning team. + +Posted every weekday the cron script invokes it, unlike the summary: these are +things somebody has to do, so a day skipped is a day nobody was asked. + +Every count links to a Bugzilla list of exactly the bugs counted. Restricted +bugs are counted and linked like any other but never named; see +`bugbot.reo_regressions`, which also holds the open regressions query and the +posting. +""" + +import datetime + +from libmozdata import utils as lmdutils + +from bugbot import logger, utils +from bugbot import reo_regressions as reo + +# Products where an unassigned high severity bug is not something to nag about, +# so they are left out of the "S2+ unassigned" bucket alone. Empty today, as the +# one exemption we have belongs to a component rather than to a whole product; +# kept so exempting a product later is a one line change. +UNASSIGNED_EXEMPT_PRODUCTS: tuple[str, ...] = () + +# The same, per component: Web Compatibility::Site Reports bugs S2 definition +# does not follow the regression severity definition. The exemption is the +# component's, not the product's — Site Reports only happens to sit under Web +# Compatibility, and the rest of that product still follows the definition. +# +# Matched on the component name alone, exactly as Bugzilla reports it, and only +# against this one bucket: an exempt bug with no severity or an unanswered +# needinfo is still stuck in the way those buckets mean. +UNASSIGNED_EXEMPT_COMPONENTS = ("Site Reports",) + +# How long a bug has to have been stuck before this message nags about it. +# Long enough that a bug filed or touched during yesterday's working day is left +# alone, short enough that nothing sits unnoticed for a second day. +# +# It ages bugs from a fixed point in the past rather than over a window, so a +# quiet weekend doesn't hide anything: a bug that went stale on Friday is still +# in Monday's message, and stays there until someone acts on it. +STUCK_HOURS = 24 + +# The channels a fix has to be uplifted to reach. A fix only reaches Beta or +# Release by being uplifted, so a burndown bug with no uplift request against the +# channel is a fix that will not ship in the version it is marked as affecting. +# +# The flag is matched by name alone, so any state of it counts as asked: pending +# (?), granted (+) and denied (-) alike. Matching only a pending request would +# put a bug back on the list the moment its uplift was approved, since the flag +# stops being pending then and the fix has yet to land, and would keep a denied +# one on the list for good. +# +# Nightly is where fixes land, so it needs no uplift and gets no burndown line. +# The order here is the order the lines appear in. +# +# A channel added here also needs a version from `utils.get_versions_from_trains()`. +# One without a version is skipped with a note in the log rather than reported. +# +# The flag name itself is built by `utils.get_flag`, so this is only the list of +# channels. That is also what makes ESR addable: its flag embeds the version +# number, which `get_flag` knows how to format and a constant here could not. +UPLIFT_CHANNELS = ("beta", "release") + +# The title leads with what makes this message different from the twice weekly +# status summary, rather than trailing it. Slack cuts a long title off in +# notification previews and the eye reads from the left, so a title starting +# "REO release regression" like the other one would be indistinguishable at a +# glance. It is also the fallback text of the message, which is what those +# previews show. +# +# Slack allows 150 characters in a header block, which this is nowhere near. +HEADING = "Action needed: REO release regressions" + +# Sits under the heading in a context block: small, grey, and read as a label on +# the message rather than as part of it. Says the message is a recurring one, so +# a reader who has not seen it before knows it is not an incident. +CADENCE = "Daily update" + +# Follows the heading. Says the one thing every line below has in common, so the +# bullets don't each have to explain themselves, and points each team at the +# sub-bullets, which is where the message asks anything of anyone. +# +# What the buckets share is that none of them is waiting on the work: each is +# waiting on an action, which is what makes the message worth sending daily and +# what separates it from the twice weekly summary of how the cycle is going. +# +# The age is given here as a round number and again on each bullet, where it is +# also said what the age is counted from, as that differs per bucket. +INTRO = ( + "These release regressions are waiting on activity and fall into the urgent " + "category. " + f"They have been pending for longer than {STUCK_HOURS} hours. " + "Please take a look where one of your teams is listed." +) + +# Shown on a day where every bucket came out empty, so a quiet day reads as good +# news rather than as the script having failed. +NOTHING_STUCK = "• Nothing needs attention" + +# What this message needs back from a bug search on top of `reo.BUG_FIELDS`: it +# ages every bug, and the three timestamps it can age one from all live on the +# bug itself, so asking for them keeps it to the same one request per version. +# Still no `summary` field -- see `reo.BUG_FIELDS`. +FIELDS = f"{reo.BUG_FIELDS},assigned_to,creation_time,last_change_time,flags" +BURNDOWN_FIELDS = f"{reo.BUG_FIELDS},cf_last_resolved" + + +def burndown_query(version: int, uplift_flag: str) -> dict: + """Build the burndown query for a version, less the bugs already asking to uplift. + + Bugs with all of the following: + - resolved as fixed + - status-firefox{version} is affected or fix-optional + - any of: + - crash, regression, leak, topcrash, assertion or dataloss keywords + - in a security group + - tracking-firefox{version} is +, ? or blocking + Bugs with any of the following are ignored: + - within one of reo.EXCLUDED_PRODUCTS + - an uplift request against the channel, in any state + + All but the last of those is bugdash's Burndown list, kept in step with + app/buglists/burndown.mjs there. Its numbering gaps at f5, f8 and f10 are + copied along with the rest, as Bugzilla ignores unused numbers, and f9 is + free now the product exclusions live at reo.EXCLUDED_PRODUCTS_SLOT. + + The f3-f7 group is what narrows "every fix still marked affected" down to the + fixes worth chasing an uplift for, and being a security bug is one of the three + ways in. It is a test of whether a bug qualifies, not of whether we can see it: + under anonymous queries that branch matched nothing, because a search never + returns a bug the requester cannot read, so it only starts contributing here. + What it adds is the fixed security bugs carrying none of those keywords and no + tracking flag — a population that would otherwise fall off the burndown despite + being perfectly visible. + + The uplift request is a flag on an attachment, and the only way a bug search + will report those is to send back every attachment with it, so it is left to + Bugzilla rather than filtered here. flagtypes.name matches the flags on a + bug's attachments as well as those on the bug itself, on name and state + together, so matching the bare name catches the request whatever became of + it. n11 negates that, leaving the fixes nobody has asked to uplift. + """ + return { + "classification": reo.CLASSIFICATIONS, + "resolution": "FIXED", + "f1": reo.status_flag(version), + "o1": "anywords", + "v1": "affected optional", + "j2": "OR", + "f2": "OP", + "f3": "keywords", + "o3": "anywords", + "v3": "crash regression leak topcrash assertion dataloss", + "f4": "bug_group", + "o4": "substring", + "v4": "sec", + "f6": reo.tracking_flag(version), + "o6": "anywordssubstr", + "v6": "+ ? blocking", + "f7": "CP", + "f11": "flagtypes.name", + "o11": "substring", + "v11": uplift_flag, + "n11": "1", + **reo.without_excluded_products(), + } + + +def stuck_since() -> datetime.datetime: + """The moment a bug has to predate to count as stuck. See STUCK_HOURS.""" + now = datetime.datetime.now(datetime.timezone.utc) + + return now - datetime.timedelta(hours=STUCK_HOURS) + + +def unassigned_exempt(bug: dict) -> bool: + """Whether a bug is exempt from the S2+ unassigned bucket. + + Exempt by product or by component, so either can be exempted on its own + without the other having to be named. + """ + return ( + bug["product"] in UNASSIGNED_EXEMPT_PRODUCTS + or bug["component"] in UNASSIGNED_EXEMPT_COMPONENTS + ) + + +def needs_assignee(bug: dict, cutoff: datetime.datetime) -> bool: + """A high severity bug nobody has taken on, aged from when it was filed. + + What counts as unassigned is `utils.is_no_assignee`, shared with the rest of + bugbot: the nobody@ placeholder, a component's `.bugs` default address, or no + assignee at all. That last pair is wider than the REO queries take it — they + count a bug parked on a component default as assigned — so this can flag a bug + bugdash would not. Nothing differed on the day it changed, but that is a fact + about that day's bug set rather than a guarantee. + + Exempt bugs are left out: an unassigned bug there is not a bug that has been + overlooked. See unassigned_exempt(). + """ + return ( + bug["severity"] in reo.HIGH_SEVERITY + and not unassigned_exempt(bug) + and utils.is_no_assignee(bug["assigned_to"]) + and lmdutils.get_date_ymd(bug["creation_time"]) < cutoff + ) + + +def needs_severity(bug: dict, cutoff: datetime.datetime) -> bool: + """A bug still waiting on a severity decision, aged from its last activity. + + Any change to the bug counts as activity, not just a triage one, so a bug + with activity is left out until it goes quiet again. There are some limitations + with this approach since the activity may be from someone outside the triage + team asking questions or adjusting metadata. + """ + return ( + bug["severity"] in reo.MISSING_SEVERITIES + and lmdutils.get_date_ymd(bug["last_change_time"]) < cutoff + ) + + +def needs_answer(bug: dict, cutoff: datetime.datetime) -> bool: + """A bug with a needinfo nobody has answered, aged from when it was requested. + + What counts as an open request is left to `utils.get_needinfo`, so this agrees + with every other rule that nags about one. The ageing is not: `get_needinfo` + filters on modification_date in whole days, and a flag's creation_date is when + the request now standing was made, so one that was answered and then asked again + is aged from the second ask rather than the first. + + Several open requests on one bug still only count the bug once, and the oldest + of them is what decides. + """ + return any( + lmdutils.get_date_ymd(flag["creation_date"]) < cutoff + for flag in utils.get_needinfo(bug) + ) + + +# The buckets, in the order they appear in the message: what makes a bug belong +# in one, the label its count goes in, and what its age is counted from. A bug +# can be in more than one, as they describe different things left undone rather +# than a state it is in. +# +# Every bucket names its own anchor because each is aged from a different +# timestamp. Left unsaid, the same "> 24 hours" on every bullet reads as one +# shared deadline, when a bug filed weeks ago and one that went quiet yesterday +# are being asked about for different reasons. +STUCK_BUCKETS = ( + (needs_assignee, "{} S2+ unassigned", "filed"), + (needs_severity, "{} missing severity", "last change"), + (needs_answer, "{} needinfo pending", "requested"), +) + + +def open_regressions(versions: dict[str, int]) -> list[dict]: + """Every open release regression across the channels, each bug listed once. + + A regression affecting Nightly usually affects Beta and Release too, so the + three queries overlap heavily: 62 hits covering 50 bugs when this was + written. Keying on the bug id merges them, which is the point of this + message — one list of what needs doing, not the same bug asked about three + times. Where two channels disagree the last query wins, but the fields the + buckets look at are all channel independent. + """ + bugs: dict[int, dict] = {} + for version in sorted(set(versions.values())): + for bug in reo.fetch_bugs(reo.regressions_query(version), FIELDS): + bugs[bug["id"]] = bug + + return list(bugs.values()) + + +def stuck_group(bugs: list[dict], label: str, anchor: str) -> str: + """Build the bullet and team sub-bullet for one bucket. + + Only the count and what it counts are linked; the restricted note and the age + that follow are left as plain text, so the blue runs as far as the thing being + claimed and no further. Building that tail here is what keeps every bullet the + same shape, the burndown lines included. + + Empty buckets return an empty string and are left out of the message, so it + stays a list of things to do rather than a scoreboard of zeros. + + Neither link gets a fallback query: the ageing is done here rather than by + Bugzilla, so there is no query URL that reproduces either count. + """ + if not bugs: + return "" + + age = f", {reo.GREATER_THAN} {STUCK_HOURS} hours since {anchor}" + + return ( + f"• {reo.bug_link(bugs, label)}{reo.restricted_note(bugs)}{age}\n" + f"{reo.SUB_BULLET}{reo.team_breakdown(bugs)}" + ) + + +def burndown_group(channel: str, version: int, cutoff: datetime.datetime) -> str: + """Build the burndown bullet for one channel, aged from when each bug was fixed. + + Unlike the other buckets this is per version rather than merged across the + channels: a fix reaches Beta and Release by separate uplifts, so the same bug + can be outstanding on one and done on the other, and each has to be asked for + against its own version. + + Nothing is subtracted for a bug fixed in the version's own cycle, as the + query only keeps bugs the version is still marked as affected by. Once a fix + is uplifted the status goes to fixed and the bug leaves the list. + """ + query = burndown_query(version, utils.get_flag(None, "approval", channel)) + bugs = [ + bug + for bug in reo.fetch_bugs(query, BURNDOWN_FIELDS) + if lmdutils.get_date_ymd(bug["cf_last_resolved"]) < cutoff + ] + label = f"{{}} Fx{version} {channel.title()} fixed with no uplift request" + + return stuck_group(bugs, label, "resolved") + + +class ReoRegressionSlackDaily(reo.ReoRegressionsRule): + """Post the release regressions that are waiting on somebody to Slack. + + No `must_run`: this one runs every day the cron script invokes it, which is + every weekday. + """ + + def description(self) -> str: + return "REO release regressions needing action posted to Slack" + + def heading(self) -> str: + return HEADING + + def blocks(self, versions: dict[str, int]) -> list[dict]: + """Build the action required message, one section per bucket. + + A header block titles the message and a context block labels it, then the + standing ask and each bucket that has anything in it follow as sections. + + The title is a header rather than bold text in a section so that it + renders at heading weight and separates the ask from the list. Header + blocks take plain text only, which is why nothing else lives in there. + """ + titles: list[dict] = [ + {"type": "header", "text": {"type": "plain_text", "text": HEADING}}, + {"type": "context", "elements": [{"type": "mrkdwn", "text": CADENCE}]}, + ] + sections = [INTRO] + + cutoff = stuck_since() + bugs = open_regressions(versions) + + groups = [ + group + for matches, label, anchor in STUCK_BUCKETS + if ( + group := stuck_group( + [bug for bug in bugs if matches(bug, cutoff)], label, anchor + ) + ) + ] + for channel in UPLIFT_CHANNELS: + version = versions.get(channel) + if version is None: + # A channel with no version to query. Skipped rather than raised, + # so adding a channel above can never be the thing that costs the + # whole message, and said out loud so it isn't a silent no-op + # either. + # + # ESR is the case that will turn up. `get_versions_from_trains` + # reports an esr version and `utils.get_flag` formats its numbered + # approval flag, so adding it here is now only a question of + # whether we want the line, not of whether the name can be built. + logger.warning("No version for %s; skipping its burndown line", channel) + continue + + if group := burndown_group(channel, version, cutoff): + groups.append(group) + + sections.extend(groups or [NOTHING_STUCK]) + + return titles + reo.to_blocks(sections) + + +if __name__ == "__main__": + ReoRegressionSlackDaily().run() diff --git a/scripts/cron_run_weekdays.sh b/scripts/cron_run_weekdays.sh index 2d21fc6e9..de2778275 100755 --- a/scripts/cron_run_weekdays.sh +++ b/scripts/cron_run_weekdays.sh @@ -196,4 +196,11 @@ python -m bugbot.rules.severity_high_performance_impact --production # Request potential missing info when a bug is moved to Core::Performance python -m bugbot.rules.moved_to_performance --production +# Post the REO release regression cycle summary to Slack +# Self-gated to Mon and Thu by the rule's own must_run +python -m bugbot.rules.reo_regression_slack --production + +# Post the REO release regressions needing action to Slack +python -m bugbot.rules.reo_regression_slack_daily --production + source ./scripts/cron_common_end.sh diff --git a/tests/rules/test_reo_regression_slack.py b/tests/rules/test_reo_regression_slack.py new file mode 100644 index 000000000..7de326f9e --- /dev/null +++ b/tests/rules/test_reo_regression_slack.py @@ -0,0 +1,69 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +import datetime + +from bugbot import reo_regressions as reo +from bugbot import utils +from bugbot.rules.reo_regression_slack import ReoRegressionSlack, regression_group + + +def test_the_rule_is_named_after_its_module(): + # Which is what `logger_extra["bugbot_rule"]` is tagged with, so a failure in + # this message is told apart from one in the daily message. + assert ReoRegressionSlack().name() == "reo_regression_slack" + + +def test_the_summary_runs_on_monday_and_thursday_only(): + rule = ReoRegressionSlack() + monday = datetime.date(2026, 8, 31) + week = [monday + datetime.timedelta(days=day) for day in range(7)] + + assert [day for day in week if rule.must_run(day)] == [ + monday, + datetime.date(2026, 9, 3), # Thursday + ] + + +def test_the_cadence_is_the_rules_own(): + # In the rule rather than in configs/rules.json, the way `missed_uplifts` and + # `workflow.p2_merge_day` decide their days, so a config entry can neither add + # a day nor take one away. + assert utils.get_config("reo_regression_slack", "must_run", None) is None + assert not ReoRegressionSlack().must_run(datetime.date(2026, 9, 1)) # Tuesday + + +def test_regression_group_notes_restricted_bugs_on_the_top_line_only(monkeypatch): + bugs = [ + {"id": 1, "severity": "S2", "groups": ["core-security-release"]}, + {"id": 2, "severity": "--", "groups": []}, + ] + monkeypatch.setattr(reo, "fetch_bugs", lambda query, fields=None: bugs) + monkeypatch.setattr(reo, "team_of", lambda bug: "Team A") + + bullet, teams, severities = regression_group(150, False, "New", by_team=True).split( + "\n" + ) + + assert bullet.endswith("|2 New Regressions> (1 restricted)") + assert "restricted" not in teams + assert "restricted" not in severities + assert "1 S2+" in severities + assert "1 missing severity" in severities + + +def test_force_bypasses_the_must_run_gate(): + parser = ReoRegressionSlack().get_args_parser() + + assert not parser.parse_args([]).force + assert parser.parse_args(["--force"]).force + + +def test_the_channel_is_a_constant_the_flag_can_override(): + parser = ReoRegressionSlack().get_args_parser() + + # No flag means the module constant, which is what the cron runs with. + assert parser.parse_args([]).channel == "" + assert parser.parse_args(["--channel", "C_TEST"]).channel == "C_TEST" + assert reo.CHANNEL.startswith("C") diff --git a/tests/rules/test_reo_regression_slack_daily.py b/tests/rules/test_reo_regression_slack_daily.py new file mode 100644 index 000000000..52061858a --- /dev/null +++ b/tests/rules/test_reo_regression_slack_daily.py @@ -0,0 +1,145 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +import datetime + +from bugbot import reo_regressions as reo +from bugbot import utils +from bugbot.rules import reo_regression_slack_daily as daily +from bugbot.rules.reo_regression_slack_daily import ReoRegressionSlackDaily + + +def test_the_rule_is_named_after_its_module(): + assert ReoRegressionSlackDaily().name() == "reo_regression_slack_daily" + + +def test_the_daily_message_is_ungated(): + # Every weekday the cron script invokes it: these are things somebody has to + # do, so a day skipped is a day nobody was asked. The summary is the one with + # a `must_run`. + rule = ReoRegressionSlackDaily() + monday = datetime.date(2026, 8, 31) + + assert all(rule.must_run(monday + datetime.timedelta(days=day)) for day in range(7)) + + +def test_the_burndown_query_drops_the_excluded_products(): + query = daily.burndown_query(150, "approval-mozilla-beta") + excluded = { + query[f"v{n}"] + for n in range( + reo.EXCLUDED_PRODUCTS_SLOT, + reo.EXCLUDED_PRODUCTS_SLOT + len(reo.EXCLUDED_PRODUCTS), + ) + if query.get(f"f{n}") == "product" + } + + assert excluded == set(reo.EXCLUDED_PRODUCTS) + + +def test_uplift_flags_come_from_get_flag(): + for channel in daily.UPLIFT_CHANNELS: + query = daily.burndown_query(150, utils.get_flag(None, "approval", channel)) + assert query["v11"] == f"approval-mozilla-{channel}" + + +def test_burndown_query_qualifies_security_bugs(): + query = daily.burndown_query(150, "approval-mozilla-beta") + + # The f2-f7 OR group narrows "still marked affected" down to "worth chasing an + # uplift for", and being a security bug is one of the three ways in. It is not + # an access filter: it only started matching anything once bugbot's key made + # those bugs visible in the first place. + assert query["j2"] == "OR" + assert (query["f4"], query["o4"], query["v4"]) == ("bug_group", "substring", "sec") + assert (query["f2"], query["f7"]) == ("OP", "CP") + + # The uplift request is negated, so what is left is the fixes nobody has asked + # to uplift. + assert query["v11"] == "approval-mozilla-beta" + assert query["n11"] == "1" + + +def test_every_search_asks_for_the_groups_field(): + # Without it no message can tell a restricted bug from a public one. + for fields in (daily.FIELDS, daily.BURNDOWN_FIELDS): + assert "groups" in fields.split(",") + + +def test_no_search_asks_for_a_bug_summary(): + # Extending `reo.BUG_FIELDS` must not be what quietly starts naming restricted + # bugs. + for fields in (daily.FIELDS, daily.BURNDOWN_FIELDS): + assert "summary" not in fields.split(",") + + +def test_the_ageing_fields_are_asked_for(): + # Each bucket ages a bug from a different timestamp, and all of them come back + # on the bug itself, which is what keeps this to one request per version. + assert {"creation_time", "last_change_time", "flags"} <= set( + daily.FIELDS.split(",") + ) + assert "cf_last_resolved" in daily.BURNDOWN_FIELDS.split(",") + + +def test_stuck_group_puts_the_note_outside_the_link_and_before_the_age(monkeypatch): + monkeypatch.setattr(reo, "team_of", lambda bug: "Team A") + bugs = [ + {"id": 1, "groups": ["core-security-release"]}, + {"id": 2, "groups": []}, + ] + + bullet, sub_bullet = daily.stuck_group(bugs, "{} S2+ unassigned", "filed").split( + "\n" + ) + + assert bullet == ( + "• (1 restricted), > 24 hours since filed" + ) + # The note belongs to the top-level bullet alone; repeated on the team line it + # would crowd out the counts that line exists for. + assert "restricted" not in sub_bullet + + +def test_stuck_group_leaves_out_an_empty_bucket(): + assert daily.stuck_group([], "{} S2+ unassigned", "filed") == "" + + +def test_bucket_predicates_read_bugzillas_trailing_z(): + # Bugzilla stamps its timestamps with a Z, which `datetime.fromisoformat` + # only learned to read in 3.11 while bugbot still supports 3.10. Ageing goes + # through libmozdata, which has always read it, and this pins that. + cutoff = datetime.datetime(2026, 8, 17, tzinfo=datetime.timezone.utc) + bug = { + "severity": "S2", + "product": "Core", + "component": "Layout", + "assigned_to": "nobody@mozilla.org", + "creation_time": "2026-08-16T23:40:15Z", + } + + assert daily.needs_assignee(bug, cutoff) + assert not daily.needs_assignee( + {**bug, "creation_time": "2026-08-18T00:00:00Z"}, cutoff + ) + + +def test_unassigned_uses_the_shared_helper(): + # utils.is_no_assignee, so a component's `.bugs` default counts as unassigned + # the way it does everywhere else in bugbot. + cutoff = datetime.datetime(2026, 8, 17, tzinfo=datetime.timezone.utc) + bug = { + "severity": "S2", + "product": "Core", + "component": "Layout", + "creation_time": "2026-08-16T23:40:15Z", + } + + assert daily.needs_assignee({**bug, "assigned_to": "nobody@mozilla.org"}, cutoff) + assert daily.needs_assignee({**bug, "assigned_to": "gfx-bugs@mozilla.bugs"}, cutoff) + assert daily.needs_assignee({**bug, "assigned_to": ""}, cutoff) + assert not daily.needs_assignee( + {**bug, "assigned_to": "someone@mozilla.com"}, cutoff + ) diff --git a/tests/test_reo_regressions.py b/tests/test_reo_regressions.py new file mode 100644 index 000000000..2593e4632 --- /dev/null +++ b/tests/test_reo_regressions.py @@ -0,0 +1,241 @@ +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at http://mozilla.org/MPL/2.0/. + +import datetime + +import pytest + +from bugbot import constants, utils +from bugbot import reo_regressions as reo + + +def test_regressions_query_without_a_split_asks_for_the_whole_set(): + query = reo.regressions_query(150) + + assert query["f1"] == "cf_status_firefox150" + assert query["v1"] == "affected" + # No condition on the previous version, so neither side of the split. + assert "f2" not in query + assert "n2" not in query + + +def test_regressions_query_new_looks_at_the_previous_version(): + query = reo.regressions_query(150, carry_over=False) + + assert query["j2"] == "OR" + assert query["f3"] == query["f4"] == query["f5"] == "cf_status_firefox149" + assert {query["v3"], query["v4"], query["v5"]} == {"unaffected", "?", "---"} + assert "n2" not in query + + +def test_regressions_query_carry_over_negates_the_whole_group(): + new = reo.regressions_query(150, carry_over=False) + carry_over = reo.regressions_query(150, carry_over=True) + + # The two partition the set, so the only difference between them is the + # negation, and it has to sit on the OP at f2 rather than on the first + # condition inside the group. + assert carry_over == {**new, "n2": "1"} + assert carry_over["f2"] == "OP" + + +@pytest.mark.parametrize("carry_over", (None, False, True)) +def test_the_regressions_query_drops_the_excluded_products(carry_over): + # The burndown query is the other one that has to, and its own test covers it. + query = reo.regressions_query(150, carry_over) + excluded = { + query[f"v{n}"] + for n in range( + reo.EXCLUDED_PRODUCTS_SLOT, + reo.EXCLUDED_PRODUCTS_SLOT + len(reo.EXCLUDED_PRODUCTS), + ) + if query.get(f"f{n}") == "product" + } + + assert excluded == set(reo.EXCLUDED_PRODUCTS) + + +def test_each_excluded_product_gets_its_own_condition(): + # Not one nowords: Bugzilla splits that value on whitespace, so + # "Developer Infrastructure" would match as two separate words and drop + # products nobody asked to exclude. + conditions = reo.without_excluded_products() + + assert len(conditions) == 3 * len(reo.EXCLUDED_PRODUCTS) + assert all( + op == "notequals" for key, op in conditions.items() if key.startswith("o") + ) + assert any( + " " in v for k, v in conditions.items() if k.startswith("v") + ), "the multi-word product is what makes the per-product split necessary" + + +def test_the_exclusion_slots_clear_every_other_slot_in_use(): + # with_severities takes 11, and the burndown's uplift flag takes 11 too, so + # the exclusions have to start above both. + query = reo.with_severities(reo.regressions_query(150), ("S1", "S2")) + used = {int(k[1:]) for k in query if k[0] in "fov" and k[1:].isdigit()} + assert reo.EXCLUDED_PRODUCTS_SLOT > max( + n for n in used if n < reo.EXCLUDED_PRODUCTS_SLOT + ) + + +def test_flag_names_come_from_get_flag(): + # utils.get_flag is the one place version numbers become flag names; these + # queries must not concatenate their own. + query = reo.regressions_query(150, carry_over=False) + + assert query["f1"] == utils.get_flag(150, "status", "release") + assert query["f8"] == utils.get_flag(150, "tracking", "release") + assert query["f3"] == utils.get_flag(149, "status", "release") + + +def test_the_shared_fields_ask_for_the_groups_field(): + # Without it no message can tell a restricted bug from a public one, and both + # rules build their field lists out of this one. + assert "groups" in reo.BUG_FIELDS.split(",") + + +def test_the_shared_fields_never_ask_for_a_bug_summary(): + # A restricted bug is counted and linked, never named, and not asking for the + # field is what makes that true of anything built on this list. + assert "summary" not in reo.BUG_FIELDS.split(",") + + +def test_restricted_note_counts_bugs_in_any_group(): + bugs = [ + {"id": 1, "groups": ["core-security-release"]}, + {"id": 2, "groups": ["mozilla-employee-confidential"]}, + {"id": 3, "groups": []}, + ] + + # Wider than the `bug_group ~ "sec"` branch in the daily rule's burndown query + # on purpose: the note explains why the linked list looks short, and any group + # does that. + assert reo.restricted_note(bugs) == " (2 restricted)" + + +@pytest.mark.parametrize("bugs", ([], [{"id": 1, "groups": []}])) +def test_restricted_note_is_silent_when_nothing_is_restricted(bugs): + assert reo.restricted_note(bugs) == "" + + +LONG_LIST = [{"id": 1000000 + i} for i in range(300)] +SHORT_URL = "https://bugzilla.mozilla.org/1a2b3c" + + +def test_bug_link_shortens_a_snapshot_that_is_too_long(monkeypatch): + # The shortener is preferred over the fallback query: it still points at + # exactly the bugs counted, where a live query can drift from the count. + monkeypatch.setattr(reo.utils, "shorten_long_bz_url", lambda url: SHORT_URL) + + link = reo.bug_link(LONG_LIST, "{} New Regressions", {"resolution": "---"}) + + assert link == f"<{SHORT_URL}|300 New Regressions>" + + +def test_bug_link_shortens_a_team_line_that_has_no_fallback(monkeypatch): + # Team lines pass no fallback, as reproducing a team as a query means listing + # all of its components, so before the shortener they lost their link entirely. + monkeypatch.setattr(reo.utils, "shorten_long_bz_url", lambda url: SHORT_URL) + + assert reo.bug_link(LONG_LIST, "{} Media") == f"<{SHORT_URL}|300 Media>" + + +def test_bug_link_rejects_the_multiline_shortener_fallback(monkeypatch): + # utils.shorten_long_bz_url answers a shortener error with the URL split over + # lines (bugbot#1402). A Slack link would end at the first newline, so that has + # to count as a failure and drop through to the query. + monkeypatch.setattr( + reo.utils, "shorten_long_bz_url", lambda url: "https://a\nhttps://b" + ) + + link = reo.bug_link(LONG_LIST, "{} New Regressions", {"resolution": "---"}) + + assert "buglist.cgi?resolution=---" in link + assert "\n" not in link + + +def test_bug_link_survives_a_shortener_that_raises(monkeypatch): + def boom(url): + raise RuntimeError("shortener down") + + monkeypatch.setattr(reo.utils, "shorten_long_bz_url", boom) + + link = reo.bug_link(LONG_LIST, "{} New Regressions", {"resolution": "---"}) + + assert "buglist.cgi?resolution=---" in link + + +def test_bug_link_is_left_unlinked_when_nothing_works(monkeypatch): + monkeypatch.setattr( + reo.utils, "shorten_long_bz_url", lambda url: "https://a\nhttps://b" + ) + + assert reo.bug_link(LONG_LIST, "{} Media") == "300 Media" + + +def test_bug_link_does_not_shorten_a_url_that_fits(monkeypatch): + def unexpected(url): + raise AssertionError("a short URL should never reach the shortener") + + monkeypatch.setattr(reo.utils, "shorten_long_bz_url", unexpected) + + assert reo.bug_link([{"id": 1}], "{} Media") == ( + "" + ) + + +def test_to_blocks_wraps_each_section_on_its_own(): + assert reo.to_blocks(["one", "two"]) == [ + {"type": "section", "text": {"type": "mrkdwn", "text": "one"}}, + {"type": "section", "text": {"type": "mrkdwn", "text": "two"}}, + ] + + +def test_to_blocks_refuses_to_post_an_overflowing_section(): + with pytest.raises(RuntimeError, match="over the 3000 limit"): + reo.to_blocks(["x" * (reo.SECTION_LIMIT + 1)]) + + +def test_block_text_reads_every_block_shape(): + blocks = [ + {"type": "header", "text": {"type": "plain_text", "text": "Title"}}, + {"type": "context", "elements": [{"type": "mrkdwn", "text": "Daily update"}]}, + {"type": "section", "text": {"type": "mrkdwn", "text": "Body"}}, + ] + + assert [reo.block_text(block) for block in blocks] == [ + "Title", + "Daily update", + "Body", + ] + + +def test_a_rule_runs_every_day_unless_it_says_otherwise(): + # The daily message is left ungated by taking this as it comes; the summary + # overrides it. + class Anything(reo.ReoRegressionsRule): + pass + + assert all( + Anything().must_run(datetime.date(2026, 8, 31) + datetime.timedelta(days=day)) + for day in range(7) + ) + + +def test_high_severity_is_the_shared_constant(): + # Shared with the rest of bugbot rather than the REO queries' ("S1", "S2"), so + # the legacy names count too. + assert reo.HIGH_SEVERITY is constants.HIGH_SEVERITY + assert {"S1", "S2", "critical", "major", "blocker"} <= reo.HIGH_SEVERITY + + +def test_with_severities_builds_a_stable_url(): + # HIGH_SEVERITY is a set, and set iteration order is not stable across + # processes, so the value has to be sorted or the URL changes run to run. + query = reo.with_severities({"resolution": "---"}, reo.HIGH_SEVERITY) + + assert query["v11"] == ", ".join(sorted(reo.HIGH_SEVERITY)) + assert query["o11"] == "anyexact" From 14305242e1674ef4c992af38a8f3e14a82dd4587 Mon Sep 17 00:00:00 2001 From: DonalMe <76789979+DonalMe@users.noreply.github.com> Date: Tue, 15 Sep 2026 11:45:11 -0400 Subject: [PATCH 3/6] Change reo_regression rules to use BzCleaner --- bugbot/reo_regressions.py | 208 +++++------------- bugbot/rules/reo_regression_slack.py | 180 ++++++++++----- bugbot/rules/reo_regression_slack_daily.py | 169 ++++++++++---- configs/rules.json | 3 + scripts/cron_run_weekdays.sh | 2 +- tests/rules/test_reo_regression_slack.py | 55 +++-- .../rules/test_reo_regression_slack_daily.py | 5 +- tests/test_reo_regressions.py | 74 ++++++- 8 files changed, 406 insertions(+), 290 deletions(-) diff --git a/bugbot/reo_regressions.py b/bugbot/reo_regressions.py index 332eedc16..cb58ddb65 100644 --- a/bugbot/reo_regressions.py +++ b/bugbot/reo_regressions.py @@ -11,11 +11,13 @@ - `bugbot.rules.reo_regression_slack_daily`, the action required message, every weekday -What lives here is mostly what both of them need: the open regressions query, -the Bugzilla search and link building, the team breakdown, the Block Kit -wrapping and the `ReoRegressionsRule` base class that runs one of them. What is -one message's own -- its heading, its cadence, its buckets -- lives in that -rule. +Both are `BzCleaner` rules, so the searches go out through `get_bz_params` and +`get_bugs`, and the run itself -- the rule's name, its arguments, its `must_run` +gate, its logging and its error handling -- is the framework's. What lives here +is what the two of them share on top of that: the open regressions query, the +Bugzilla link building, the team breakdown, the Block Kit wrapping and the +posting. What is one message's own -- its heading, its cadence, its buckets -- +lives in that rule. A few things here have one caller today, and each says so where it is defined. They are kept here because of what they are rather than who uses them: a @@ -25,7 +27,7 @@ second caller would mean. Shaped after `bugbot.multinaggers` and `bugbot.topcrash`: a module here holding -what rules under `bugbot/rules` share, base class included. +what rules under `bugbot/rules` share. Restricted bugs are counted in the totals and included in the links like any other, but never named: no message prints a bug summary, which is the same line @@ -38,13 +40,10 @@ import argparse import datetime import functools -import os -import sys from collections.abc import Collection -from libmozdata.bugzilla import Bugzilla - -from bugbot import logger, logger_extra, slack, utils +from bugbot import logger, slack, utils +from bugbot.bzcleaner import BzCleaner from bugbot.components import ComponentName, fetch_component_teams # Shared with the rest of bugbot rather than redeclared as ("S1", "S2") the way the @@ -306,33 +305,6 @@ def team_of(bug: dict) -> str: return component_teams().get(ComponentName.from_bug(bug)) or UNKNOWN_TEAM -def fetch_bugs(query: dict, fields: str = BUG_FIELDS) -> list[dict]: - """Return the requested fields of every bug matching a query. - - Fetching the bugs rather than asking for count_only is what lets the severity - and team breakdowns be derived from one request, and lets each count link to - the exact bugs behind it. - - `fields` defaults to the shared list, which is everything the cycle summary - asks of a bug; the daily rule passes its own, longer one. - - libmozdata pages the search itself — counting first, then walking the results in - chunks — but only for a query carrying none of count_only, limit, order or - offset, so none of those may be added here. It also attaches bugbot's API key, - which is the whole reason these rules see restricted bugs at all. - """ - bugs: list[dict] = [] - - Bugzilla( - {**query, "include_fields": fields}, - bughandler=lambda bug, data: data.append(bug), - bugdata=bugs, - timeout=utils.get_config("common", "bz_query_timeout"), - ).get_data().wait() - - return bugs - - def query_url(query: dict) -> str: """A Bugzilla URL that re-runs a query, so its results change over time.""" return utils.get_bz_search_url(query) @@ -492,121 +464,59 @@ def block_text(block: dict) -> str: return block["text"]["text"] -class ReoRegressionsRule: - """Base for the rules that post a REO release regression message to Slack. - - A subclass says what its message is called, when it runs and what is in it; - building and posting it is the same either way and happens here. +def add_channel_argument(parser: argparse.ArgumentParser) -> None: + """Add the flag that sends a run's message somewhere other than CHANNEL. - Not a `BzCleaner`: these rules run several queries rather than one, report - counts rather than a table of bugs, write nothing to Bugzilla, and have to - post on a quiet day to say so -- where `BzCleaner` sends nothing when there - are no bugs, and its cache would suppress a bug that must reappear until it - is fixed. The conventions that do apply are borrowed rather than reinvented: - `description`, `must_run(date)` and a `name` taken from the module file all - mean what they mean there, `--production` is the flag that makes a run real, - `logger_extra["bugbot_rule"]` tags the log, and a failure is logged and - swallowed so one message can't fail the whole cron job. + Added through `BzCleaner.add_custom_arguments`, so a rule keeps every + standard flag -- `--production`, `--date` -- and gains this one. """ + parser.add_argument( + "--channel", + action="store", + default="", + help=( + f"Slack channel ID to post to, overriding {CHANNEL}. Useful to shake " + "the message out somewhere else without editing the code." + ), + ) + + +def versions_to_report() -> dict[str, int]: + """The current version of each channel, with what was read written to the log. + + From the trains API rather than through `BzCleaner.init_versions`: + `utils.get_checked_versions` returns nothing on merge day, and + `has_enough_data` would then skip the run on exactly the day both messages + have their own wording for. + """ + versions = utils.get_versions_from_trains() + logger.info( + "Reporting Firefox %s release / %s beta / %s nightly", + versions["release"], + versions["beta"], + versions["nightly"], + ) - def __init__(self) -> None: - self.__rule_name__ = self._rule_name() + return versions - def _rule_name(self) -> str: - """The rule name, taken from the module file as `BzCleaner` takes it.""" - module = sys.modules[self.__class__.__module__] - module_file = module.__file__ - assert module_file is not None - return os.path.splitext(os.path.basename(module_file))[0] +def post_message( + rule: BzCleaner, channel: str, heading: str, blocks: list[dict] +) -> None: + """Post a rule's message to Slack, or print it when the run isn't for real. - def name(self) -> str: - """Get the rule name""" - return self.__rule_name__ + `heading` is the message's notification fallback text, which is what a + client that cannot render blocks shows instead of them. - def description(self) -> str: - """Get the description for the help""" - return "" - - def heading(self) -> str: - """The message's title, which is also its notification fallback text.""" - raise NotImplementedError - - def must_run(self, date: datetime.date) -> bool: - """Check if the rule must run for this date""" - return True - - def blocks(self, versions: dict[str, int]) -> list[dict]: - """Build the message as Block Kit blocks.""" - raise NotImplementedError - - def get_args_parser(self) -> argparse.ArgumentParser: - """Get the arguments from the command line""" - parser = argparse.ArgumentParser(description=self.description()) - parser.add_argument( - "--production", - dest="dryrun", - action="store_false", - help=( - "If the flag is not passed, just build the message and print it to " - "the console without posting it to Slack" - ), - ) - parser.add_argument( - "--channel", - action="store", - default="", - help=( - f"Slack channel ID to post to, overriding {CHANNEL}. Useful to shake " - "the message out somewhere else without editing the code." - ), - ) - parser.add_argument( - "--force", - action="store_true", - help=( - "Post even on a day the rule's own must_run would skip. No effect " - "on a rule that posts whenever it is invoked" - ), - ) - - return parser - - def run(self) -> None: - """Run the rule""" - logger_extra["bugbot_rule"] = self.name() - logger.info("Run rule %s", self.name()) - - args = self.get_args_parser().parse_args() - - try: - if not args.force and not self.must_run(utc_today()): - logger.info( - "%s: not a day this message runs on; --force overrides", - self.name(), - ) - return - - versions = utils.get_versions_from_trains() - blocks = self.blocks(versions) - - if args.dryrun: - print("DRY RUN: message not posted.\n") - for block in blocks: - print(block_text(block)) - return - - slack.post_to_slack(args.channel or CHANNEL, self.heading(), blocks=blocks) - logger.info( - "Rule %s posted for Firefox %s / %s / %s", - self.name(), - versions["release"], - versions["beta"], - versions["nightly"], - ) - except Exception: - # Logged and swallowed, as `BzCleaner.run` does: `bugbot.log --send` - # mails the digest at the end of the cron run, so the failure is seen - # without a non-zero exit tripping the ERR trap and failing the whole - # job over one message. - logger.exception("Rule %s", self.name()) + A dry run prints what it would have posted, so `--production` means here what + it means for every other rule. `test_mode` is honoured alongside it for the + reason `triage_owner_rotations` honours it: a test run must reach nobody. + """ + if rule.dryrun or rule.test_mode: + print("DRY RUN: message not posted.\n") + for block in blocks: + print(block_text(block)) + return + + slack.post_to_slack(channel, heading, blocks=blocks) + logger.info("Rule %s posted to %s", rule.name(), channel) diff --git a/bugbot/rules/reo_regression_slack.py b/bugbot/rules/reo_regression_slack.py index 4a2ba3851..26d159d9a 100644 --- a/bugbot/rules/reo_regression_slack.py +++ b/bugbot/rules/reo_regression_slack.py @@ -24,9 +24,11 @@ `reo_regression_slack_daily`. """ +import argparse import datetime import functools import re +from typing import Any import requests from libmozdata import utils as lmdutils @@ -34,17 +36,7 @@ from bugbot import reo_regressions as reo from bugbot import utils - -# The days this message is posted on. In the rule rather than in -# `configs/rules.json` because the cadence is part of what the message is: it -# reports how a cycle is going rather than what has just changed, and it says so -# in its own wording, so a day added to it in configuration would not make the -# message it produces any more true. `missed_uplifts` and `workflow.p2_merge_day` -# decide their days in the rule for the same reason. -# -# Twice a week rather than daily: the counts move slowly, and a summary that -# arrives every morning stops being read. -MUST_RUN_DAYS = ("Mon", "Thu") +from bugbot.bzcleaner import Bug, BzCleaner, BzParams, EmailData RELEASE_PAGE_URL = "https://whattrainisitnow.com/release/?version={}" @@ -172,60 +164,134 @@ def cycle_countdown(version: int, channel: str) -> str: return f'{label} {end:%Y-%m-%d} in {days} {utils.plural("working day", days)}' -def regression_group( - version: int, carry_over: bool, label: str, by_team: bool = False -) -> str: - """Build the bullet and severity sub-bullets for one bug list. - - The list is fetched once and split by severity and team here, rather than - asking Bugzilla for each subset, so the sub-bullets are guaranteed to be - part of the count above them. +class ReoRegressionSlack(BzCleaner): + """Post the state of this cycle's open release regressions to Slack. - Bug lists that are empty are left out entirely rather than reported as a - zero, so a quiet channel is short instead of a wall of "0". Returns an - empty string when there are no bugs at all. + A `BzCleaner` that reports to Slack instead of by email: the searches, the + arguments, the `must_run` gate and the error handling are all the + framework's, and `get_email_data` posts the message and returns nothing to + mail. The days it runs on are `must_run` in `configs/rules.json`. """ - query = reo.regressions_query(version, carry_over) - bugs = reo.fetch_bugs(query) - if not bugs: - return "" - - link = reo.bug_link(bugs, f"{{}} {label} Regressions", query) - lines = [f"• {link}{reo.restricted_note(bugs)}"] - - if by_team: - lines.append(reo.SUB_BULLET + reo.team_breakdown(bugs)) - - severity_counts = [] - for severities, template in ( - (reo.HIGH_SEVERITY, "{} S2+"), - (reo.MISSING_SEVERITIES, "{} missing severity"), - ): - subset = [bug for bug in bugs if bug["severity"] in severities] - if subset: - severity_counts.append( - reo.bug_link(subset, template, reo.with_severities(query, severities)) - ) - if severity_counts: - lines.append(reo.SUB_BULLET + ", ".join(severity_counts)) + # Where the message goes. A `--channel` run overrides it, so this is the + # channel the cron posts to; see `parse_custom_arguments`. + channel = reo.CHANNEL - return "\n".join(lines) + def description(self) -> str: + return "REO release regression cycle summary posted to Slack" + def all_include_fields(self) -> bool: + # The fields a search asks for are `reo.BUG_FIELDS` and nothing else. + # `BzCleaner` would otherwise add `summary` to every query, which is the + # one field no message here prints -- see `reo.restricted_note`. + return True + + def has_default_products(self) -> bool: + # The query is scoped by classification, as bugdash's REO queries are; + # the default product list would report a different bug set. + return False + + def filter_no_nag_keyword(self) -> bool: + # This message counts bugs rather than nagging about them, and a + # [no-nag] bug is still one the cycle is carrying. Dropping those would + # put the counts out of step with the REO tab. + return False + + def add_custom_arguments(self, parser: argparse.ArgumentParser) -> None: + reo.add_channel_argument(parser) + + def parse_custom_arguments(self, args: argparse.Namespace) -> None: + self.channel = args.channel or reo.CHANNEL + + def get_bz_params(self, date: str) -> BzParams: + """The query the running `get_bugs()` call is for. See `fetch_bugs`.""" + return self.params + + def bughandler(self, bug: Bug, data: dict[str, Any]) -> None: + """Keep every field of the bug, keyed by its id. + + `BzCleaner`'s own handler reduces a bug to the columns of an email + table, its summary included. This message reports counts and + breakdowns, so it needs the fields it asked for and none of the rest. + """ + data[str(bug["id"])] = bug + + def fetch_bugs(self, query: dict, fields: str = reo.BUG_FIELDS) -> list[dict]: + """Run one of this rule's queries through `BzCleaner`'s search path. + + Several queries per run -- two per channel -- each one set here and read + back by `get_bz_params`, the way `warn_regressed_by` steps through its + two. Going through `get_bugs` is what attaches bugbot's API key, which + is the whole reason this sees restricted bugs, along with the query + timeout and the paging. libmozdata pages a search itself -- counting + first, then walking the results in chunks -- but only for a query + carrying none of count_only, limit, order or offset, so no query here + may add one. + + Fetching the bugs rather than asking for count_only is what lets the + severity and team breakdowns be derived from one request, and lets each + count link to the exact bugs behind it. + """ + self.params = {**query, "include_fields": fields} + + return list(self.get_bugs().values()) + + def regression_group( + self, version: int, carry_over: bool, label: str, by_team: bool = False + ) -> str: + """Build the bullet and severity sub-bullets for one bug list. + + The list is fetched once and split by severity and team here, rather than + asking Bugzilla for each subset, so the sub-bullets are guaranteed to be + part of the count above them. + + Bug lists that are empty are left out entirely rather than reported as a + zero, so a quiet channel is short instead of a wall of "0". Returns an + empty string when there are no bugs at all. + """ + query = reo.regressions_query(version, carry_over) + bugs = self.fetch_bugs(query) + if not bugs: + return "" + + link = reo.bug_link(bugs, f"{{}} {label} Regressions", query) + lines = [f"• {link}{reo.restricted_note(bugs)}"] + + if by_team: + lines.append(reo.SUB_BULLET + reo.team_breakdown(bugs)) + + severity_counts = [] + for severities, template in ( + (reo.HIGH_SEVERITY, "{} S2+"), + (reo.MISSING_SEVERITIES, "{} missing severity"), + ): + subset = [bug for bug in bugs if bug["severity"] in severities] + if subset: + severity_counts.append( + reo.bug_link( + subset, template, reo.with_severities(query, severities) + ) + ) -class ReoRegressionSlack(reo.ReoRegressionsRule): - """Post the state of this cycle's open release regressions to Slack.""" + if severity_counts: + lines.append(reo.SUB_BULLET + ", ".join(severity_counts)) - def description(self) -> str: - return "REO release regression cycle summary posted to Slack" + return "\n".join(lines) - def heading(self) -> str: - return HEADING + def get_email_data(self, date: str) -> EmailData: + """Post the message, and give `send_email` nothing to send. - def must_run(self, date: datetime.date) -> bool: - weekdays = utils.get_weekdays() + The report is the Slack message rather than an email, and an empty list + is what stops one being sent -- the same way `security_affected_versions` + runs the pipeline for the needinfos it posts and mails no summary. The + "No data" line `send_email` then logs is about that email, not about the + message, which has been posted by the time it is written. + """ + reo.post_message( + self, self.channel, HEADING, self.blocks(reo.versions_to_report()) + ) - return any(weekdays[day] == date.weekday() for day in MUST_RUN_DAYS) + return [] def blocks(self, versions: dict[str, int]) -> list[dict]: """Build the cycle summary as Block Kit sections, one per bug list.""" @@ -244,8 +310,8 @@ def blocks(self, versions: dict[str, int]) -> list[dict]: groups = [ group for group in ( - regression_group(version, False, "New", by_team=True), - regression_group(version, True, "Carry Over"), + self.regression_group(version, False, "New", by_team=True), + self.regression_group(version, True, "Carry Over"), ) if group ] diff --git a/bugbot/rules/reo_regression_slack_daily.py b/bugbot/rules/reo_regression_slack_daily.py index ac9a60604..ac7503c07 100644 --- a/bugbot/rules/reo_regression_slack_daily.py +++ b/bugbot/rules/reo_regression_slack_daily.py @@ -21,12 +21,15 @@ posting. """ +import argparse import datetime +from typing import Any from libmozdata import utils as lmdutils from bugbot import logger, utils from bugbot import reo_regressions as reo +from bugbot.bzcleaner import Bug, BzCleaner, BzParams, EmailData # Products where an unassigned high severity bug is not something to nag about, # so they are left out of the "S2+ unassigned" bucket alone. Empty today, as the @@ -267,24 +270,6 @@ def needs_answer(bug: dict, cutoff: datetime.datetime) -> bool: ) -def open_regressions(versions: dict[str, int]) -> list[dict]: - """Every open release regression across the channels, each bug listed once. - - A regression affecting Nightly usually affects Beta and Release too, so the - three queries overlap heavily: 62 hits covering 50 bugs when this was - written. Keying on the bug id merges them, which is the point of this - message — one list of what needs doing, not the same bug asked about three - times. Where two channels disagree the last query wins, but the fields the - buckets look at are all channel independent. - """ - bugs: dict[int, dict] = {} - for version in sorted(set(versions.values())): - for bug in reo.fetch_bugs(reo.regressions_query(version), FIELDS): - bugs[bug["id"]] = bug - - return list(bugs.values()) - - def stuck_group(bugs: list[dict], label: str, anchor: str) -> str: """Build the bullet and team sub-bullet for one bucket. @@ -310,41 +295,133 @@ def stuck_group(bugs: list[dict], label: str, anchor: str) -> str: ) -def burndown_group(channel: str, version: int, cutoff: datetime.datetime) -> str: - """Build the burndown bullet for one channel, aged from when each bug was fixed. +class ReoRegressionSlackDaily(BzCleaner): + """Post the release regressions that are waiting on somebody to Slack. - Unlike the other buckets this is per version rather than merged across the - channels: a fix reaches Beta and Release by separate uplifts, so the same bug - can be outstanding on one and done on the other, and each has to be asked for - against its own version. + A `BzCleaner` that reports to Slack instead of by email: the searches, the + arguments and the error handling are all the framework's, and + `get_email_data` posts the message and returns nothing to mail. - Nothing is subtracted for a bug fixed in the version's own cycle, as the - query only keeps bugs the version is still marked as affected by. Once a fix - is uplifted the status goes to fixed and the bug leaves the list. + No `must_run` entry in `configs/rules.json`: this one runs every day the + cron script invokes it, which is every weekday. The twice weekly summary, + `reo_regression_slack`, is the one with a cadence of its own. """ - query = burndown_query(version, utils.get_flag(None, "approval", channel)) - bugs = [ - bug - for bug in reo.fetch_bugs(query, BURNDOWN_FIELDS) - if lmdutils.get_date_ymd(bug["cf_last_resolved"]) < cutoff - ] - label = f"{{}} Fx{version} {channel.title()} fixed with no uplift request" - return stuck_group(bugs, label, "resolved") + # Where the message goes. A `--channel` run overrides it, so this is the + # channel the cron posts to; see `parse_custom_arguments`. + channel = reo.CHANNEL + def description(self) -> str: + return "REO release regressions needing action posted to Slack" -class ReoRegressionSlackDaily(reo.ReoRegressionsRule): - """Post the release regressions that are waiting on somebody to Slack. + def all_include_fields(self) -> bool: + # The fields a search asks for are FIELDS and BURNDOWN_FIELDS and + # nothing else. `BzCleaner` would otherwise add `summary` to every + # query, which is the one field no message here prints. + return True - No `must_run`: this one runs every day the cron script invokes it, which is - every weekday. - """ + def has_default_products(self) -> bool: + # Both queries are scoped by classification, as bugdash's are; the + # default product list would report a different bug set. + return False - def description(self) -> str: - return "REO release regressions needing action posted to Slack" + def filter_no_nag_keyword(self) -> bool: + # [no-nag] is a request not to mail a bug's assignee about it. This + # message names teams rather than people and is read by the release + # managers chasing the work, so dropping those bugs would hide work + # that still has to be done. + return False + + def add_custom_arguments(self, parser: argparse.ArgumentParser) -> None: + reo.add_channel_argument(parser) + + def parse_custom_arguments(self, args: argparse.Namespace) -> None: + self.channel = args.channel or reo.CHANNEL + + def get_bz_params(self, date: str) -> BzParams: + """The query the running `get_bugs()` call is for. See `fetch_bugs`.""" + return self.params + + def bughandler(self, bug: Bug, data: dict[str, Any]) -> None: + """Keep every field of the bug, keyed by its id. + + `BzCleaner`'s own handler reduces a bug to the columns of an email + table, its summary included. This message ages every bug and counts it, + so it needs the fields it asked for and none of the rest. + """ + data[str(bug["id"])] = bug + + def fetch_bugs(self, query: dict, fields: str = FIELDS) -> list[dict]: + """Run one of this rule's queries through `BzCleaner`'s search path. + + Several queries per run -- one per version, plus one per burndown line + -- each one set here and read back by `get_bz_params`, the way + `warn_regressed_by` steps through its two. Going through `get_bugs` is + what attaches bugbot's API key, which is the whole reason this sees + restricted bugs, along with the query timeout and the paging. libmozdata + pages a search itself -- counting first, then walking the results in + chunks -- but only for a query carrying none of count_only, limit, order + or offset, so no query here may add one. + """ + self.params = {**query, "include_fields": fields} + + return list(self.get_bugs().values()) + + def open_regressions(self, versions: dict[str, int]) -> list[dict]: + """Every open release regression across the channels, each bug listed once. + + A regression affecting Nightly usually affects Beta and Release too, so + the three queries overlap heavily: 62 hits covering 50 bugs when this + was written. Keying on the bug id merges them, which is the point of + this message — one list of what needs doing, not the same bug asked + about three times. Where two channels disagree the last query wins, but + the fields the buckets look at are all channel independent. + """ + bugs: dict[int, dict] = {} + for version in sorted(set(versions.values())): + for bug in self.fetch_bugs(reo.regressions_query(version)): + bugs[bug["id"]] = bug + + return list(bugs.values()) + + def burndown_group( + self, channel: str, version: int, cutoff: datetime.datetime + ) -> str: + """Build the burndown bullet for one channel, aged from when each bug was fixed. + + Unlike the other buckets this is per version rather than merged across the + channels: a fix reaches Beta and Release by separate uplifts, so the same bug + can be outstanding on one and done on the other, and each has to be asked for + against its own version. + + Nothing is subtracted for a bug fixed in the version's own cycle, as the + query only keeps bugs the version is still marked as affected by. Once a fix + is uplifted the status goes to fixed and the bug leaves the list. + """ + query = burndown_query(version, utils.get_flag(None, "approval", channel)) + bugs = [ + bug + for bug in self.fetch_bugs(query, BURNDOWN_FIELDS) + if lmdutils.get_date_ymd(bug["cf_last_resolved"]) < cutoff + ] + label = f"{{}} Fx{version} {channel.title()} fixed with no uplift request" + + return stuck_group(bugs, label, "resolved") + + def get_email_data(self, date: str) -> EmailData: + """Post the message, and give `send_email` nothing to send. + + The report is the Slack message rather than an email, and an empty list + is what stops one being sent -- the same way `security_affected_versions` + runs the pipeline for the needinfos it posts and mails no summary. The + "No data" line `send_email` then logs is about that email, not about the + message, which has been posted by the time it is written. + """ + reo.post_message( + self, self.channel, HEADING, self.blocks(reo.versions_to_report()) + ) - def heading(self) -> str: - return HEADING + return [] def blocks(self, versions: dict[str, int]) -> list[dict]: """Build the action required message, one section per bucket. @@ -363,7 +440,7 @@ def blocks(self, versions: dict[str, int]) -> list[dict]: sections = [INTRO] cutoff = stuck_since() - bugs = open_regressions(versions) + bugs = self.open_regressions(versions) groups = [ group @@ -389,7 +466,7 @@ def blocks(self, versions: dict[str, int]) -> list[dict]: logger.warning("No version for %s; skipping its burndown line", channel) continue - if group := burndown_group(channel, version, cutoff): + if group := self.burndown_group(channel, version, cutoff): groups.append(group) sections.extend(groups or [NOTHING_STUCK]) diff --git a/configs/rules.json b/configs/rules.json index 630e561dd..1878b9e57 100644 --- a/configs/rules.json +++ b/configs/rules.json @@ -490,5 +490,8 @@ }, "topcrash_notify": { "additional_receivers": ["rm"] + }, + "reo_regression_slack": { + "must_run": ["Mon", "Thu"] } } diff --git a/scripts/cron_run_weekdays.sh b/scripts/cron_run_weekdays.sh index de2778275..5b6677755 100755 --- a/scripts/cron_run_weekdays.sh +++ b/scripts/cron_run_weekdays.sh @@ -197,7 +197,7 @@ python -m bugbot.rules.severity_high_performance_impact --production python -m bugbot.rules.moved_to_performance --production # Post the REO release regression cycle summary to Slack -# Self-gated to Mon and Thu by the rule's own must_run +# Gated to Mon and Thu by its must_run in configs/rules.json python -m bugbot.rules.reo_regression_slack --production # Post the REO release regressions needing action to Slack diff --git a/tests/rules/test_reo_regression_slack.py b/tests/rules/test_reo_regression_slack.py index 7de326f9e..e02f7b206 100644 --- a/tests/rules/test_reo_regression_slack.py +++ b/tests/rules/test_reo_regression_slack.py @@ -6,7 +6,7 @@ from bugbot import reo_regressions as reo from bugbot import utils -from bugbot.rules.reo_regression_slack import ReoRegressionSlack, regression_group +from bugbot.rules.reo_regression_slack import ReoRegressionSlack def test_the_rule_is_named_after_its_module(): @@ -17,21 +17,30 @@ def test_the_rule_is_named_after_its_module(): def test_the_summary_runs_on_monday_and_thursday_only(): rule = ReoRegressionSlack() - monday = datetime.date(2026, 8, 31) + monday = datetime.datetime(2026, 8, 31) week = [monday + datetime.timedelta(days=day) for day in range(7)] assert [day for day in week if rule.must_run(day)] == [ monday, - datetime.date(2026, 9, 3), # Thursday + datetime.datetime(2026, 9, 3), # Thursday ] -def test_the_cadence_is_the_rules_own(): - # In the rule rather than in configs/rules.json, the way `missed_uplifts` and - # `workflow.p2_merge_day` decide their days, so a config entry can neither add - # a day nor take one away. - assert utils.get_config("reo_regression_slack", "must_run", None) is None - assert not ReoRegressionSlack().must_run(datetime.date(2026, 9, 1)) # Tuesday +def test_the_cadence_comes_from_the_rule_config(): + # `BzCleaner.must_run` reads it, so the days are configuration rather than an + # override here. Twice a week rather than daily: the counts move slowly, and a + # summary that arrives every morning stops being read. + assert utils.get_config("reo_regression_slack", "must_run") == ["Mon", "Thu"] + + +def test_a_run_can_be_pointed_at_another_day(): + # `BzCleaner`'s own --date, which is how the Mon/Thu gate is exercised without + # waiting for a Monday. + rule = ReoRegressionSlack() + args = rule.get_args_parser().parse_args(["-D", "2026-09-01"]) + + assert not rule.must_run(datetime.datetime(2026, 9, 1)) # Tuesday + assert args.date == "2026-09-01" def test_regression_group_notes_restricted_bugs_on_the_top_line_only(monkeypatch): @@ -39,12 +48,13 @@ def test_regression_group_notes_restricted_bugs_on_the_top_line_only(monkeypatch {"id": 1, "severity": "S2", "groups": ["core-security-release"]}, {"id": 2, "severity": "--", "groups": []}, ] - monkeypatch.setattr(reo, "fetch_bugs", lambda query, fields=None: bugs) + rule = ReoRegressionSlack() + monkeypatch.setattr(rule, "fetch_bugs", lambda query, fields=None: bugs) monkeypatch.setattr(reo, "team_of", lambda bug: "Team A") - bullet, teams, severities = regression_group(150, False, "New", by_team=True).split( - "\n" - ) + bullet, teams, severities = rule.regression_group( + 150, False, "New", by_team=True + ).split("\n") assert bullet.endswith("|2 New Regressions> (1 restricted)") assert "restricted" not in teams @@ -53,17 +63,14 @@ def test_regression_group_notes_restricted_bugs_on_the_top_line_only(monkeypatch assert "1 missing severity" in severities -def test_force_bypasses_the_must_run_gate(): - parser = ReoRegressionSlack().get_args_parser() - - assert not parser.parse_args([]).force - assert parser.parse_args(["--force"]).force - - def test_the_channel_is_a_constant_the_flag_can_override(): - parser = ReoRegressionSlack().get_args_parser() + rule = ReoRegressionSlack() + parser = rule.get_args_parser() - # No flag means the module constant, which is what the cron runs with. - assert parser.parse_args([]).channel == "" - assert parser.parse_args(["--channel", "C_TEST"]).channel == "C_TEST" + # No flag means the module constant, which is what the cron posts to. + rule.parse_custom_arguments(parser.parse_args([])) + assert rule.channel == reo.CHANNEL assert reo.CHANNEL.startswith("C") + + rule.parse_custom_arguments(parser.parse_args(["--channel", "C_TEST"])) + assert rule.channel == "C_TEST" diff --git a/tests/rules/test_reo_regression_slack_daily.py b/tests/rules/test_reo_regression_slack_daily.py index 52061858a..8260ca97f 100644 --- a/tests/rules/test_reo_regression_slack_daily.py +++ b/tests/rules/test_reo_regression_slack_daily.py @@ -17,10 +17,11 @@ def test_the_rule_is_named_after_its_module(): def test_the_daily_message_is_ungated(): # Every weekday the cron script invokes it: these are things somebody has to # do, so a day skipped is a day nobody was asked. The summary is the one with - # a `must_run`. + # a `must_run` in configs/rules.json. rule = ReoRegressionSlackDaily() - monday = datetime.date(2026, 8, 31) + monday = datetime.datetime(2026, 8, 31) + assert utils.get_config("reo_regression_slack_daily", "must_run") is None assert all(rule.must_run(monday + datetime.timedelta(days=day)) for day in range(7)) diff --git a/tests/test_reo_regressions.py b/tests/test_reo_regressions.py index 2593e4632..801f81022 100644 --- a/tests/test_reo_regressions.py +++ b/tests/test_reo_regressions.py @@ -2,12 +2,17 @@ # License, v. 2.0. If a copy of the MPL was not distributed with this file, # You can obtain one at http://mozilla.org/MPL/2.0/. -import datetime - import pytest from bugbot import constants, utils from bugbot import reo_regressions as reo +from bugbot.bzcleaner import BzCleaner +from bugbot.rules.reo_regression_slack import ReoRegressionSlack +from bugbot.rules.reo_regression_slack_daily import ReoRegressionSlackDaily + +# Both messages, for the behaviour they share as `BzCleaner` rules. What one of +# them does on its own is in its own test file. +RULES = (ReoRegressionSlack, ReoRegressionSlackDaily) def test_regressions_query_without_a_split_asks_for_the_whole_set(): @@ -213,17 +218,64 @@ def test_block_text_reads_every_block_shape(): ] -def test_a_rule_runs_every_day_unless_it_says_otherwise(): - # The daily message is left ungated by taking this as it comes; the summary - # overrides it. - class Anything(reo.ReoRegressionsRule): - pass - - assert all( - Anything().must_run(datetime.date(2026, 8, 31) + datetime.timedelta(days=day)) - for day in range(7) +@pytest.mark.parametrize("rule_class", RULES) +def test_both_messages_are_bzcleaner_rules(rule_class): + # The searches, the arguments, the must_run gate and the error handling are + # the framework's; what these two add is where the report goes. + assert isinstance(rule_class(), BzCleaner) + + +@pytest.mark.parametrize("rule_class", RULES) +def test_a_query_goes_out_as_the_rule_built_it(rule_class): + rule = rule_class() + params = {**reo.regressions_query(150), "include_fields": reo.BUG_FIELDS} + rule.amend_bzparams(params, []) + + # No `summary`: a restricted bug is counted and linked, never named. + assert params["include_fields"] == reo.BUG_FIELDS + # No default product list, no [no-nag] exclusion and no group filter. The + # query is bugdash's, and what it matches is what gets counted. + assert "product" not in params + assert "[no-nag]" not in params.values() + assert "bug_group" not in params.values() + + +@pytest.mark.parametrize("rule_class", RULES) +def test_neither_message_caches_the_bugs_it_reports(rule_class): + # A bug belongs in these messages until somebody acts on it, so the cache + # that keeps other rules from repeating themselves has to stay off. It is by + # default -- `max_days_in_cache` is -1 -- and this is what would catch a + # configs/rules.json entry turning it on. + rule = rule_class() + rule.cache.set_dry_run(False) # as a --production run does + + assert rule.max_days_in_cache() < 1 + assert 1234 not in rule.cache + + +@pytest.mark.parametrize("rule_class", RULES) +def test_the_report_is_a_slack_message_rather_than_an_email(rule_class, monkeypatch): + posted = [] + rule = rule_class() + rule.dryrun = False + rule.test_mode = False + monkeypatch.setattr(reo, "versions_to_report", lambda: {}) + monkeypatch.setattr(rule, "blocks", lambda versions: ["a block"]) + monkeypatch.setattr( + reo.slack, + "post_to_slack", + lambda channel, text, blocks=None: posted.append((channel, text, blocks)) + or "1.0", ) + # The empty list is what stops `send_email` sending anything. + assert rule.get_email_data("today") == [] + + (channel, text, blocks), *rest = posted + assert not rest + assert (channel, blocks) == (reo.CHANNEL, ["a block"]) + assert text, "the message needs its notification fallback text" + def test_high_severity_is_the_shared_constant(): # Shared with the rest of bugbot rather than the REO queries' ("S1", "S2"), so From 5def16374439f8dcf2623a3290854252250943d1 Mon Sep 17 00:00:00 2001 From: DonalMe <76789979+DonalMe@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:23:36 -0400 Subject: [PATCH 4/6] Move reo_regressions logic into the individual rules --- bugbot/reo_regressions.py | 522 ------------------ bugbot/rules/reo_regression_slack.py | 464 +++++++++++++++- bugbot/rules/reo_regression_slack_daily.py | 425 ++++++++++++-- tests/rules/test_reo_regression_slack.py | 8 +- .../rules/test_reo_regression_slack_daily.py | 11 +- tests/test_reo_regressions.py | 293 ---------- 6 files changed, 838 insertions(+), 885 deletions(-) delete mode 100644 bugbot/reo_regressions.py delete mode 100644 tests/test_reo_regressions.py diff --git a/bugbot/reo_regressions.py b/bugbot/reo_regressions.py deleted file mode 100644 index cb58ddb65..000000000 --- a/bugbot/reo_regressions.py +++ /dev/null @@ -1,522 +0,0 @@ -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this file, -# You can obtain one at http://mozilla.org/MPL/2.0/. - -"""Shared parts of the rules that post the REO release regressions to Slack. - -Two rules are built on this, both over the bug set behind the REO tab of -https://bugdash.moz.tools/: - -- `bugbot.rules.reo_regression_slack`, the cycle summary, Mon and Thu -- `bugbot.rules.reo_regression_slack_daily`, the action required message, every - weekday - -Both are `BzCleaner` rules, so the searches go out through `get_bz_params` and -`get_bugs`, and the run itself -- the rule's name, its arguments, its `must_run` -gate, its logging and its error handling -- is the framework's. What lives here -is what the two of them share on top of that: the open regressions query, the -Bugzilla link building, the team breakdown, the Block Kit wrapping and the -posting. What is one message's own -- its heading, its cadence, its buckets -- -lives in that rule. - -A few things here have one caller today, and each says so where it is defined. -They are kept here because of what they are rather than who uses them: a -condition on a query this module builds, or a fact about Slack's markup. Moving -each one to whichever rule happens to call it would leave the next reader -looking in two files for one vocabulary, and moving it back is what adding the -second caller would mean. - -Shaped after `bugbot.multinaggers` and `bugbot.topcrash`: a module here holding -what rules under `bugbot/rules` share. - -Restricted bugs are counted in the totals and included in the links like any -other, but never named: no message prints a bug summary, which is the same line -`BzCleaner.get_summary` draws. The top-level bullet says how many of its count -are restricted, because a reader without access opens the link and finds a -shorter list than the number they clicked on, and the note is what explains the -gap. See `restricted_note`. -""" - -import argparse -import datetime -import functools -from collections.abc import Collection - -from bugbot import logger, slack, utils -from bugbot.bzcleaner import BzCleaner -from bugbot.components import ComponentName, fetch_component_teams - -# Shared with the rest of bugbot rather than redeclared as ("S1", "S2") the way the -# REO queries have it. It also carries the pre-S1 names — critical maps to S1, major -# and blocker to S2 (see `constants.OLD_SEVERITY_MAP`) — so an old bug that never had -# its severity restated still lands in the S2+ counts instead of quietly missing from -# them. That is a wider net than bugdash casts, so these numbers can run slightly -# ahead of the REO tab's. -# -# Re-exported here rather than imported by each rule, so that reasoning is written -# down once for both messages. -from bugbot.constants import HIGH_SEVERITY # noqa: F401 - -# The channel both rules post to. Here rather than in `configs/rules.json` -# because it is not a secret, and because changing where an unattended recurring -# message lands should take a code review -- the same reasoning `frontend_triage` -# gives for keeping its component list in code. The bot token is the part that is -# a secret, and that stays in `configs/config.json`. -# -# TEMPORARY: this is currently #tmp-dm-test, a scratch channel for shaking the -# port out. It has to be pointed at the real REO channel before either message is -# meant for anyone to read. -CHANNEL = "C0BLP0WUBED" - -BZ_BUGLIST_URL = "https://bugzilla.mozilla.org/buglist.cgi" - -# Every Bugzilla classification except Graveyard, which holds the ~100 retired -# products. Same list bugdash's REO queries use. -# -# "Developer Infrastructure" appearing here and in EXCLUDED_PRODUCTS is not a -# contradiction: the classification also holds Firefox Build System, Conduit and -# Tree Management, which stay in scope, and only the product of the same name is -# dropped. -CLASSIFICATIONS = [ - "Client Software", - "Components", - "Developer Infrastructure", - "Other", - "Server Software", -] - -# The severity that means no triage decision has been made yet. Bugs are filtered -# on this locally, so the value has to be exactly what Bugzilla reports in a bug's -# severity field, which is case sensitive and not always what the same value looks -# like in a search: "N/A" comes back from the API where a query matches it as -# "n/a". Only "--" counts as missing here; N/A is a decision, not the absence of -# one. -MISSING_SEVERITIES = ("--",) - -# Products dropped from every query, so their bugs reach neither message and no -# bucket in them. Excluded at the query rather than per bucket, so a product here -# is out of the cycle summary, all the daily buckets and the burndown lines alike. -# -# Not the `exclude_products` key some rules carry in `configs/rules.json`: that one -# subtracts from `BzCleaner`'s default product list, which these classification -# scoped queries never use, so the name would mean something different here. -EXCLUDED_PRODUCTS = ("Testing", "Developer Infrastructure") - -# Where the product exclusions are numbered from in a boolean chart. Above every -# slot either query uses -- `regressions_query` here and `burndown_query` in the -# daily rule -- including the 11 `with_severities` takes. -EXCLUDED_PRODUCTS_SLOT = 12 - -# For a component with no team_name, or one missing from the mapping entirely. -# Every component had a team when this was written, so this is only a guard -# against silently dropping bugs out of the per-team line. -UNKNOWN_TEAM = "Unknown team" - -# A Slack section block holds at most 3000 characters. -SECTION_LIMIT = 3000 - -# Above this length a snapshot URL is shortened, and failing that swapped for the -# query URL or dropped entirely -- see `bug_link`. Keeps one very long bug list -# from pushing a section over SECTION_LIMIT. -MAX_SNAPSHOT_URL = 2000 - -# Slack renders this back as >. Sending the character itself would work where it -# is used now, but it ends a link's label at the first > and opens a blockquote at -# the start of a line, so a label or bullet reworded around it would break in ways -# that are easy to miss. The entity is never wrong. -# -# The daily rule's "> 24 hours" is the only use today. It is here rather than -# there because it is a fact about Slack's markup, like SUB_BULLET below, and not -# about that message. -GREATER_THAN = ">" - -# Slack has no nested lists in message text, so indent sub-bullets by hand. -# Four plain spaces; if Slack ever collapses them, non-breaking spaces (U+00A0) -# are the fix. -SUB_BULLET = " ◦ " - -# What a bug search has to come back with for either message. The cycle summary -# needs no more than this and takes it as `fetch_bugs`'s default; the daily rule -# extends it, as it ages every bug and the timestamps it ages from live on the -# bug itself. -# -# `groups` is in here rather than in one of those extensions: it is how a bug is -# known to be restricted, and every message counts those. See `restricted_note`. -# -# No `summary` field, here or in either extension. That is the line neither -# message crosses, and the same one `BzCleaner.get_summary` draws. -BUG_FIELDS = "id,severity,product,component,groups" - - -def utc_today() -> datetime.date: - """Today in UTC: milestone dates are UTC and the cron host may not be.""" - return datetime.datetime.now(datetime.timezone.utc).date() - - -def without_excluded_products(slot: int = EXCLUDED_PRODUCTS_SLOT) -> dict: - """Chart conditions dropping EXCLUDED_PRODUCTS, a numbered slot per product. - - One ANDed notequals per product rather than a single nowords: Bugzilla splits - a nowords value on whitespace, so "Developer Infrastructure" would be matched - as the two words separately and drop products nobody asked to exclude. - """ - conditions: dict = {} - for offset, product in enumerate(EXCLUDED_PRODUCTS): - number = slot + offset - conditions |= { - f"f{number}": "product", - f"o{number}": "notequals", - f"v{number}": product, - } - - return conditions - - -def status_flag(version: int) -> str: - """The status flag for a Firefox version, e.g. `cf_status_firefox150`. - - Built by `utils.get_flag` rather than concatenated: that is the one place - version numbers become flag names anywhere in bugbot. Its channel argument only - changes the name for ESR, and every version these queries run over is a desktop - one, so which channel the version happens to be on doesn't enter into it. - """ - return utils.get_flag(version, "status", "release") - - -def tracking_flag(version: int) -> str: - """The tracking flag for a Firefox version, e.g. `cf_tracking_firefox150`. - - See `status_flag` for why this goes through `utils.get_flag`. - """ - return utils.get_flag(version, "tracking", "release") - - -def regressions_query(version: int, carry_over: bool | None = None) -> dict: - """Build the open regressions query for a version. - - Bugs with all of the following: - - regression keyword - - open (unresolved) - - status-firefox{version} is affected - Bugs with any of the following are ignored: - - tracking-firefox{version} is - - - stalled or intermittent-failure keywords - - within one of EXCLUDED_PRODUCTS - - carry_over adds a condition on the previous version, splitting that set in - two. False keeps the bugs where status-firefox{version - 1} is one of - unaffected, ? or ---, so they regressed during this cycle; True negates it, - leaving the ones that were already there. The two therefore partition every - open regression affecting the version, and the default of None asks for that - whole set instead of one side of it. - - Nothing here filters on `bug_group`: an authenticated search returns every bug - the key can see, so restricted regressions arrive on their own. - - Field numbering is Bugzilla's boolean charts: f/o/v are the field, operator - and value for a numbered condition, OP and CP open and close a group, j sets - how a group joins (OR here, AND otherwise) and n negates. The gaps at f7 and - f9 are harmless, as Bugzilla ignores unused numbers: f7 comes from bugdash, - f9 from the product exclusions moving to EXCLUDED_PRODUCTS_SLOT. - """ - query = { - "classification": CLASSIFICATIONS, - "keywords": "regression", - "keywords_type": "allwords", - "resolution": "---", - "f1": status_flag(version), - "o1": "equals", - "v1": "affected", - "f8": tracking_flag(version), - "o8": "notequals", - "v8": "-", - "f10": "keywords", - "o10": "nowordssubstr", - "v10": "stalled,intermittent-failure", - **without_excluded_products(), - } - - if carry_over is None: - return query - - # Conditions are matched up by their number, so leaving these out above and - # adding them here changes nothing but the order they appear in the URL. - previous = status_flag(version - 1) - query |= { - "f2": "OP", - "j2": "OR", - "f3": previous, - "o3": "equals", - "v3": "unaffected", - "f4": previous, - "o4": "equals", - "v4": "?", - "f5": previous, - "o5": "equals", - "v5": "---", - "f6": "CP", - } - - if carry_over: - # n2 attaches to the OP at f2, so it negates the whole f3-f5 group rather - # than just the first condition in it. - query["n2"] = "1" - - return query - - -def with_severities(query: dict, severities: Collection[str]) -> dict: - """Narrow a query to some severities, for a link that stays live. - - The counts themselves are filtered locally, so this is only needed to build a - URL when a bug list is too long to link by id. Slot 11 is free: the regressions - query leaves it unused, and EXCLUDED_PRODUCTS_SLOT starts above it. - - The cycle summary is the only caller today. It is here rather than there - because it edits `regressions_query`'s chart, and which slot it may take can - only be answered next to the slots that query and the daily rule's burndown - query have already spoken for. - - Sorted so the same set of severities always produces the same URL: the order a - set iterates in is not stable from one process to the next, and `HIGH_SEVERITY` - is a set. - """ - return { - **query, - "f11": "bug_severity", - "o11": "anyexact", - "v11": ", ".join(sorted(severities)), - } - - -@functools.cache -def component_teams() -> dict[ComponentName, str]: - """Map every (product, component) to the team that owns it. - - team_name is a Bugzilla field on components, the same one bugdash's Teams - filter uses. One request covers every product, around 120KB for 2000-odd - components, which is why it's cached for the life of the run. - """ - return fetch_component_teams() - - -def team_of(bug: dict) -> str: - """The team owning a bug's component.""" - return component_teams().get(ComponentName.from_bug(bug)) or UNKNOWN_TEAM - - -def query_url(query: dict) -> str: - """A Bugzilla URL that re-runs a query, so its results change over time.""" - return utils.get_bz_search_url(query) - - -def snapshot_url(bugs: list[dict]) -> str: - """A Bugzilla URL listing exactly these bugs, as bugdash's bug lists do. - - Linking the bug ids rather than the query means the list still matches the - count in the message when it is read days later. order=bug_list keeps - Bugzilla showing them in the order given rather than re-sorting. - - Built by hand rather than through `utils.get_bz_search_url` so the separators - stay as commas: percent-encoded they would triple in length, and the length is - what `MAX_SNAPSHOT_URL` is measuring. - - Restricted bugs are in here with everything else. A reader without access gets - a shorter list than the count that linked them here, which is what the - "(n restricted)" note on the bullet is for. - """ - ids = ",".join(str(bug["id"]) for bug in bugs) - - return f"{BZ_BUGLIST_URL}?bug_id={ids}&order=bug_list" - - -def shortened_url(url: str) -> str | None: - """A short Bugzilla URL for a long one, or None if it couldn't be shortened. - - `utils.shorten_long_bz_url` answers a shortener error by returning the URL - split across several lines (bugbot#1402). Harmless in an email, useless in a - Slack link, which would end at the first newline — so a multi-line answer is - treated as a failure here rather than posted. - - Any other failure is swallowed for the same reason: the count is the message - and the link is a convenience, so a shortener that is down should cost the link - and nothing more. - """ - try: - short = utils.shorten_long_bz_url(url) - except Exception: - logger.exception("Could not shorten a Bugzilla URL") - return None - - if "\n" in short or len(short) > MAX_SNAPSHOT_URL: - return None - - return short - - -def bug_link( - bugs: list[dict], label_template: str, fallback_query: dict | None = None -) -> str: - """Format a non-empty bug list as a Slack link labelled with its count. - - label_template is formatted with the count, e.g. "{} New Regressions". - - A snapshot URL that comes out too long is shortened, which keeps the link - pointing at exactly the bugs counted. Failing that it falls back to - fallback_query, which is a live query and so can drift from the count beside - it, and failing that the count is left unlinked. Team lines pass no fallback, - as reproducing a team as a query means listing all its components, so before - the shortener they lost their link entirely. - - Callers are expected to skip empty lists: an empty bug_id would link to a - broken list, and a count of zero is left out of the message anyway. - """ - label = label_template.format(len(bugs)) - snapshot = snapshot_url(bugs) - - if len(snapshot) <= MAX_SNAPSHOT_URL: - return f"<{snapshot}|{label}>" - - url = shortened_url(snapshot) - if url is None and fallback_query is not None: - url = query_url(fallback_query) - - if url is None: - return label - - return f"<{url}|{label}>" - - -def restricted_note(bugs: list[dict]) -> str: - """Say how many of a bug list are restricted, or nothing when none are. - - A bug is restricted when it is in any group at all, not only a security one: - the note exists to explain why the linked list looks shorter than the count to - a reader without access, and that gap opens for an employee-confidential or - partner group just as it does for `core-security`. That is a wider test than - the `bug_group ~ "sec"` branch in the daily rule's burndown query, which is - asking a different question — whether a fix is worth chasing, not whether it is - readable. - - Deliberately plain text rather than part of the link label, so the blue runs as - far as the thing being counted and no further, and deliberately only used on the - top-level bullets: repeated on every severity and team sub-bullet it would say - little and crowd out the counts that are the point of those lines. - """ - count = sum(1 for bug in bugs if bug.get("groups")) - if not count: - return "" - - return f" ({count} restricted)" - - -def team_breakdown(bugs: list[dict]) -> str: - """Count the bugs owned by each team, busiest team first. - - Every team is listed rather than just the top few, so that the line works - as a nudge to each team that owns something. - """ - by_team: dict[str, list[dict]] = {} - for bug in bugs: - by_team.setdefault(team_of(bug), []).append(bug) - - ranked = sorted(by_team.items(), key=lambda item: (-len(item[1]), item[0])) - - return ", ".join(bug_link(team_bugs, f"{{}} {team}") for team, team_bugs in ranked) - - -def to_blocks(sections: list[str]) -> list[dict]: - """Wrap the sections of a message as Block Kit sections. - - Slack silently splits a message whose text runs past about 4000 characters - into several messages, which is what happened when every count linked to a - full query URL. Snapshot URLs brought the total well under that, but each - section block gets its own 3000 character allowance, so keeping the sections - means a busier cycle can't start splitting the message again. - - A section that does overflow raises rather than posting something malformed. - The team breakdown is the part that could get there, at roughly 90 characters - per team; capping or splitting it is the fix if that ever fires. - """ - for section in sections: - if len(section) > SECTION_LIMIT: - raise RuntimeError( - f"Slack section block is {len(section)} characters, over the " - f"{SECTION_LIMIT} limit:\n{section[:200]}..." - ) - - return [ - {"type": "section", "text": {"type": "mrkdwn", "text": section}} - for section in sections - ] - - -def block_text(block: dict) -> str: - """The text of any block, for printing a message instead of posting it. - - Section and header blocks keep their text in one place and context blocks in - a list of elements, so a dry run has to handle both rather than assume the - shape of the blocks it was handed. - """ - if "elements" in block: - return " ".join(element["text"] for element in block["elements"]) - - return block["text"]["text"] - - -def add_channel_argument(parser: argparse.ArgumentParser) -> None: - """Add the flag that sends a run's message somewhere other than CHANNEL. - - Added through `BzCleaner.add_custom_arguments`, so a rule keeps every - standard flag -- `--production`, `--date` -- and gains this one. - """ - parser.add_argument( - "--channel", - action="store", - default="", - help=( - f"Slack channel ID to post to, overriding {CHANNEL}. Useful to shake " - "the message out somewhere else without editing the code." - ), - ) - - -def versions_to_report() -> dict[str, int]: - """The current version of each channel, with what was read written to the log. - - From the trains API rather than through `BzCleaner.init_versions`: - `utils.get_checked_versions` returns nothing on merge day, and - `has_enough_data` would then skip the run on exactly the day both messages - have their own wording for. - """ - versions = utils.get_versions_from_trains() - logger.info( - "Reporting Firefox %s release / %s beta / %s nightly", - versions["release"], - versions["beta"], - versions["nightly"], - ) - - return versions - - -def post_message( - rule: BzCleaner, channel: str, heading: str, blocks: list[dict] -) -> None: - """Post a rule's message to Slack, or print it when the run isn't for real. - - `heading` is the message's notification fallback text, which is what a - client that cannot render blocks shows instead of them. - - A dry run prints what it would have posted, so `--production` means here what - it means for every other rule. `test_mode` is honoured alongside it for the - reason `triage_owner_rotations` honours it: a test run must reach nobody. - """ - if rule.dryrun or rule.test_mode: - print("DRY RUN: message not posted.\n") - for block in blocks: - print(block_text(block)) - return - - slack.post_to_slack(channel, heading, blocks=blocks) - logger.info("Rule %s posted to %s", rule.name(), channel) diff --git a/bugbot/rules/reo_regression_slack.py b/bugbot/rules/reo_regression_slack.py index 26d159d9a..a8294a7ac 100644 --- a/bugbot/rules/reo_regression_slack.py +++ b/bugbot/rules/reo_regression_slack.py @@ -17,8 +17,11 @@ Nightly get a working day countdown to the end of their cycle. Every count links to a Bugzilla list of exactly the bugs counted. Restricted -bugs are counted and linked like any other but never named; see -`bugbot.reo_regressions`, which also holds the queries and the posting. +bugs are counted in the totals and included in the links like any other, but +never named: no message prints a bug summary, which is the same line +`BzCleaner.get_summary` draws. The top-level bullet says how many of its count +are restricted, because a reader without access opens the link and finds a +shorter list than the number they clicked on. See `restricted_note`. The regressions that need chasing rather than counting are the other rule, `reo_regression_slack_daily`. @@ -28,15 +31,37 @@ import datetime import functools import re +from collections.abc import Collection from typing import Any import requests from libmozdata import utils as lmdutils from libmozdata.fx_trains import FirefoxTrains -from bugbot import reo_regressions as reo -from bugbot import utils +from bugbot import logger, slack, utils from bugbot.bzcleaner import Bug, BzCleaner, BzParams, EmailData +from bugbot.components import ComponentName, fetch_component_teams + +# Shared with the rest of bugbot rather than redeclared as ("S1", "S2") the way the +# REO queries have it. It also carries the pre-S1 names — critical maps to S1, major +# and blocker to S2 (see `constants.OLD_SEVERITY_MAP`) — so an old bug that never had +# its severity restated still lands in the S2+ counts instead of quietly missing from +# them. That is a wider net than bugdash casts, so these numbers can run slightly +# ahead of the REO tab's. +from bugbot.constants import HIGH_SEVERITY + +# The channel this rule posts to. Here rather than in `configs/rules.json` +# because it is not a secret, and because changing where an unattended recurring +# message lands should take a code review -- the same reasoning `frontend_triage` +# gives for keeping its component list in code. The bot token is the part that is +# a secret, and that stays in `configs/config.json`. +# +# TEMPORARY: this is currently #tmp-dm-test, a scratch channel for shaking the +# port out. It has to be pointed at the real REO channel before this message is +# meant for anyone to read. +CHANNEL = "C0BLP0WUBED" + +BZ_BUGLIST_URL = "https://bugzilla.mozilla.org/buglist.cgi" RELEASE_PAGE_URL = "https://whattrainisitnow.com/release/?version={}" @@ -46,6 +71,68 @@ # `bz_query_timeout`, which is far longer. HTTP_TIMEOUT_SECONDS = 15 +# Every Bugzilla classification except Graveyard, which holds the ~100 retired +# products. Same list bugdash's REO queries use. +# +# "Developer Infrastructure" appearing here and in EXCLUDED_PRODUCTS is not a +# contradiction: the classification also holds Firefox Build System, Conduit and +# Tree Management, which stay in scope, and only the product of the same name is +# dropped. +CLASSIFICATIONS = [ + "Client Software", + "Components", + "Developer Infrastructure", + "Other", + "Server Software", +] + +# The severity that means no triage decision has been made yet. Bugs are filtered +# on this locally, so the value has to be exactly what Bugzilla reports in a bug's +# severity field, which is case sensitive and not always what the same value looks +# like in a search: "N/A" comes back from the API where a query matches it as +# "n/a". Only "--" counts as missing here; N/A is a decision, not the absence of +# one. +MISSING_SEVERITIES = ("--",) + +# Products dropped from every query, so their bugs reach no bucket in this +# message. +# +# Not the `exclude_products` key some rules carry in `configs/rules.json`: that one +# subtracts from `BzCleaner`'s default product list, which these classification +# scoped queries never use, so the name would mean something different here. +EXCLUDED_PRODUCTS = ("Testing", "Developer Infrastructure") + +# Where the product exclusions are numbered from in a boolean chart. Above every +# slot `regressions_query` uses, including the 11 `with_severities` takes. +EXCLUDED_PRODUCTS_SLOT = 12 + +# For a component with no team_name, or one missing from the mapping entirely. +# Every component had a team when this was written, so this is only a guard +# against silently dropping bugs out of the per-team line. +UNKNOWN_TEAM = "Unknown team" + +# A Slack section block holds at most 3000 characters. +SECTION_LIMIT = 3000 + +# Above this length a snapshot URL is shortened, and failing that swapped for the +# query URL or dropped entirely -- see `bug_link`. Keeps one very long bug list +# from pushing a section over SECTION_LIMIT. +MAX_SNAPSHOT_URL = 2000 + +# Slack has no nested lists in message text, so indent sub-bullets by hand. +# Four plain spaces; if Slack ever collapses them, non-breaking spaces (U+00A0) +# are the fix. +SUB_BULLET = " ◦ " + +# What a bug search has to come back with for this message. +# +# `groups` is how a bug is known to be restricted, and the message counts those. +# See `restricted_note`. +# +# No `summary` field. That is the line this message does not cross, and the same +# one `BzCleaner.get_summary` draws. +BUG_FIELDS = "id,severity,product,component,groups" + # Stands in for a milestone key, as the last beta is numbered differently from # one version to the next (beta_10 for 154, beta_5 under the 2 week cadence). LAST_BETA = "last_beta" @@ -75,6 +162,308 @@ NOTHING_TO_REPORT = "• No open release regressions" +def utc_today() -> datetime.date: + """Today in UTC: milestone dates are UTC and the cron host may not be.""" + return lmdutils.get_date_ymd("today").date() + + +def without_excluded_products(slot: int = EXCLUDED_PRODUCTS_SLOT) -> dict: + """Chart conditions dropping EXCLUDED_PRODUCTS, a numbered slot per product. + + One ANDed notequals per product rather than a single nowords: Bugzilla splits + a nowords value on whitespace, so "Developer Infrastructure" would be matched + as the two words separately and drop products nobody asked to exclude. + """ + conditions: dict = {} + for offset, product in enumerate(EXCLUDED_PRODUCTS): + number = slot + offset + conditions |= { + f"f{number}": "product", + f"o{number}": "notequals", + f"v{number}": product, + } + + return conditions + + +def regressions_query(version: int, carry_over: bool | None = None) -> dict: + """Build the open regressions query for a version. + + Bugs with all of the following: + - regression keyword + - open (unresolved) + - status-firefox{version} is affected + Bugs with any of the following are ignored: + - tracking-firefox{version} is - + - stalled or intermittent-failure keywords + - within one of EXCLUDED_PRODUCTS + + carry_over adds a condition on the previous version, splitting that set in + two. False keeps the bugs where status-firefox{version - 1} is one of + unaffected, ? or ---, so they regressed during this cycle; True negates it, + leaving the ones that were already there. The two therefore partition every + open regression affecting the version, and the default of None asks for that + whole set instead of one side of it. + + Nothing here filters on `bug_group`: an authenticated search returns every bug + the key can see, so restricted regressions arrive on their own. + + Field numbering is Bugzilla's boolean charts: f/o/v are the field, operator + and value for a numbered condition, OP and CP open and close a group, j sets + how a group joins (OR here, AND otherwise) and n negates. The gaps at f7 and + f9 are harmless, as Bugzilla ignores unused numbers: f7 comes from bugdash, + f9 from the product exclusions moving to EXCLUDED_PRODUCTS_SLOT. + """ + query = { + "classification": CLASSIFICATIONS, + "keywords": "regression", + "keywords_type": "allwords", + "resolution": "---", + "f1": utils.get_flag(version, "status", "release"), + "o1": "equals", + "v1": "affected", + "f8": utils.get_flag(version, "tracking", "release"), + "o8": "notequals", + "v8": "-", + "f10": "keywords", + "o10": "nowordssubstr", + "v10": "stalled,intermittent-failure", + **without_excluded_products(), + } + + if carry_over is None: + return query + + # Conditions are matched up by their number, so leaving these out above and + # adding them here changes nothing but the order they appear in the URL. + previous = utils.get_flag(version - 1, "status", "release") + query |= { + "f2": "OP", + "j2": "OR", + "f3": previous, + "o3": "equals", + "v3": "unaffected", + "f4": previous, + "o4": "equals", + "v4": "?", + "f5": previous, + "o5": "equals", + "v5": "---", + "f6": "CP", + } + + if carry_over: + # n2 attaches to the OP at f2, so it negates the whole f3-f5 group rather + # than just the first condition in it. + query["n2"] = "1" + + return query + + +def with_severities(query: dict, severities: Collection[str]) -> dict: + """Narrow a query to some severities, for a link that stays live. + + The counts themselves are filtered locally, so this is only needed to build a + URL when a bug list is too long to link by id. Slot 11 is free: the regressions + query leaves it unused, and EXCLUDED_PRODUCTS_SLOT starts above it. + + Sorted so the same set of severities always produces the same URL: the order a + set iterates in is not stable from one process to the next, and `HIGH_SEVERITY` + is a set. + """ + return { + **query, + "f11": "bug_severity", + "o11": "anyexact", + "v11": ", ".join(sorted(severities)), + } + + +def snapshot_url(bugs: list[dict]) -> str: + """A Bugzilla URL listing exactly these bugs, as bugdash's bug lists do. + + Linking the bug ids rather than the query means the list still matches the + count in the message when it is read days later. order=bug_list keeps + Bugzilla showing them in the order given rather than re-sorting. + + Built by hand rather than through `utils.get_bz_search_url` so the separators + stay as commas: percent-encoded they would triple in length, and the length is + what `MAX_SNAPSHOT_URL` is measuring. + + Restricted bugs are in here with everything else. A reader without access gets + a shorter list than the count that linked them here, which is what the + "(n restricted)" note on the bullet is for. + """ + ids = ",".join(str(bug["id"]) for bug in bugs) + + return f"{BZ_BUGLIST_URL}?bug_id={ids}&order=bug_list" + + +def shortened_url(url: str) -> str | None: + """A short Bugzilla URL for a long one, or None if it couldn't be shortened. + + `utils.shorten_long_bz_url` answers a shortener error by returning the URL + split across several lines (bugbot#1402). Harmless in an email, useless in a + Slack link, which would end at the first newline — so a multi-line answer is + treated as a failure here rather than posted. + + Any other failure is swallowed for the same reason: the count is the message + and the link is a convenience, so a shortener that is down should cost the link + and nothing more. + """ + try: + short = utils.shorten_long_bz_url(url) + except Exception: + logger.exception("Could not shorten a Bugzilla URL") + return None + + if "\n" in short or len(short) > MAX_SNAPSHOT_URL: + return None + + return short + + +def bug_link( + bugs: list[dict], label_template: str, fallback_query: dict | None = None +) -> str: + """Format a non-empty bug list as a Slack link labelled with its count. + + label_template is formatted with the count, e.g. "{} New Regressions". + + A snapshot URL that comes out too long is shortened, which keeps the link + pointing at exactly the bugs counted. Failing that it falls back to + fallback_query, which is a live query and so can drift from the count beside + it, and failing that the count is left unlinked. Team lines pass no fallback, + as reproducing a team as a query means listing all its components. + + Callers are expected to skip empty lists: an empty bug_id would link to a + broken list, and a count of zero is left out of the message anyway. + """ + label = label_template.format(len(bugs)) + snapshot = snapshot_url(bugs) + + if len(snapshot) <= MAX_SNAPSHOT_URL: + return f"<{snapshot}|{label}>" + + url = shortened_url(snapshot) + if url is None and fallback_query is not None: + url = utils.get_bz_search_url(fallback_query) + + if url is None: + return label + + return f"<{url}|{label}>" + + +@functools.cache +def component_teams() -> dict[ComponentName, str]: + """Map every (product, component) to the team that owns it. + + team_name is a Bugzilla field on components, the same one bugdash's Teams + filter uses. One request covers every product, around 120KB for 2000-odd + components, which is why it's cached for the life of the run. + """ + return fetch_component_teams() + + +def team_of(bug: dict) -> str: + """The team owning a bug's component.""" + return component_teams().get(ComponentName.from_bug(bug)) or UNKNOWN_TEAM + + +def team_breakdown(bugs: list[dict]) -> str: + """Count the bugs owned by each team, busiest team first. + + Every team is listed rather than just the top few, so that the line works + as a nudge to each team that owns something. + """ + by_team: dict[str, list[dict]] = {} + for bug in bugs: + by_team.setdefault(team_of(bug), []).append(bug) + + ranked = sorted(by_team.items(), key=lambda item: (-len(item[1]), item[0])) + + return ", ".join(bug_link(team_bugs, f"{{}} {team}") for team, team_bugs in ranked) + + +def restricted_note(bugs: list[dict]) -> str: + """Say how many of a bug list are restricted, or nothing when none are. + + A bug is restricted when it is in any group at all, not only a security one: + the note exists to explain why the linked list looks shorter than the count to + a reader without access, and that gap opens for an employee-confidential or + partner group just as it does for `core-security`. + + Deliberately plain text rather than part of the link label, so the blue runs as + far as the thing being counted and no further, and deliberately only used on the + top-level bullets: repeated on every severity and team sub-bullet it would say + little and crowd out the counts that are the point of those lines. + """ + count = sum(1 for bug in bugs if bug.get("groups")) + if not count: + return "" + + return f" ({count} restricted)" + + +def to_blocks(sections: list[str]) -> list[dict]: + """Wrap the sections of a message as Block Kit sections. + + Slack silently splits a message whose text runs past about 4000 characters + into several messages, which is what happened when every count linked to a + full query URL. Snapshot URLs brought the total well under that, but each + section block gets its own 3000 character allowance, so keeping the sections + means a busier cycle can't start splitting the message again. + + A section that does overflow raises rather than posting something malformed. + The team breakdown is the part that could get there, at roughly 90 characters + per team; capping or splitting it is the fix if that ever fires. + """ + for section in sections: + if len(section) > SECTION_LIMIT: + raise RuntimeError( + f"Slack section block is {len(section)} characters, over the " + f"{SECTION_LIMIT} limit:\n{section[:200]}..." + ) + + return [ + {"type": "section", "text": {"type": "mrkdwn", "text": section}} + for section in sections + ] + + +def block_text(block: dict) -> str: + """The text of any block, for printing a message instead of posting it. + + Section and header blocks keep their text in one place and context blocks in + a list of elements, so a dry run has to handle both rather than assume the + shape of the blocks it was handed. + """ + if "elements" in block: + return " ".join(element["text"] for element in block["elements"]) + + return block["text"]["text"] + + +def versions_to_report() -> dict[str, int]: + """The current version of each channel, with what was read written to the log. + + From the trains API rather than through `BzCleaner.init_versions`: + `utils.get_checked_versions` returns nothing on merge day, and + `has_enough_data` would then skip the run on exactly the day this message + has its own wording for. + """ + versions = utils.get_versions_from_trains() + logger.info( + "Reporting Firefox %s release / %s beta / %s nightly", + versions["release"], + versions["beta"], + versions["nightly"], + ) + + return versions + + @functools.cache def wellness_days() -> frozenset[datetime.date]: """Fetch the days off that don't count as working days. @@ -100,7 +489,7 @@ def work_days_until(end: datetime.date) -> int: countdowns on the release pages: weekends, wellness days and the current day are all left out. """ - today = reo.utc_today() + today = utc_today() days = (end - today).days if days <= 0: return 0 @@ -149,7 +538,7 @@ def cycle_countdown(version: int, channel: str) -> str: cycle, milestone = CYCLE_ENDS[channel] label = f"End of {cycle}" end = milestone_date(release_schedule(version), milestone) - today = reo.utc_today() + today = utc_today() if end < today: return f"{cycle} cycle finished" @@ -175,15 +564,15 @@ class ReoRegressionSlack(BzCleaner): # Where the message goes. A `--channel` run overrides it, so this is the # channel the cron posts to; see `parse_custom_arguments`. - channel = reo.CHANNEL + channel = CHANNEL def description(self) -> str: return "REO release regression cycle summary posted to Slack" def all_include_fields(self) -> bool: - # The fields a search asks for are `reo.BUG_FIELDS` and nothing else. + # The fields a search asks for are `BUG_FIELDS` and nothing else. # `BzCleaner` would otherwise add `summary` to every query, which is the - # one field no message here prints -- see `reo.restricted_note`. + # one field this message does not print -- see `restricted_note`. return True def has_default_products(self) -> bool: @@ -198,10 +587,18 @@ def filter_no_nag_keyword(self) -> bool: return False def add_custom_arguments(self, parser: argparse.ArgumentParser) -> None: - reo.add_channel_argument(parser) + parser.add_argument( + "--channel", + action="store", + default="", + help=( + f"Slack channel ID to post to, overriding {CHANNEL}. Useful to " + "shake the message out somewhere else without editing the code." + ), + ) def parse_custom_arguments(self, args: argparse.Namespace) -> None: - self.channel = args.channel or reo.CHANNEL + self.channel = args.channel or CHANNEL def get_bz_params(self, date: str) -> BzParams: """The query the running `get_bugs()` call is for. See `fetch_bugs`.""" @@ -216,7 +613,7 @@ def bughandler(self, bug: Bug, data: dict[str, Any]) -> None: """ data[str(bug["id"])] = bug - def fetch_bugs(self, query: dict, fields: str = reo.BUG_FIELDS) -> list[dict]: + def fetch_bugs(self, query: dict, fields: str = BUG_FIELDS) -> list[dict]: """Run one of this rule's queries through `BzCleaner`'s search path. Several queries per run -- two per channel -- each one set here and read @@ -249,35 +646,52 @@ def regression_group( zero, so a quiet channel is short instead of a wall of "0". Returns an empty string when there are no bugs at all. """ - query = reo.regressions_query(version, carry_over) + query = regressions_query(version, carry_over) bugs = self.fetch_bugs(query) if not bugs: return "" - link = reo.bug_link(bugs, f"{{}} {label} Regressions", query) - lines = [f"• {link}{reo.restricted_note(bugs)}"] + link = bug_link(bugs, f"{{}} {label} Regressions", query) + lines = [f"• {link}{restricted_note(bugs)}"] if by_team: - lines.append(reo.SUB_BULLET + reo.team_breakdown(bugs)) + lines.append(SUB_BULLET + team_breakdown(bugs)) severity_counts = [] for severities, template in ( - (reo.HIGH_SEVERITY, "{} S2+"), - (reo.MISSING_SEVERITIES, "{} missing severity"), + (HIGH_SEVERITY, "{} S2+"), + (MISSING_SEVERITIES, "{} missing severity"), ): subset = [bug for bug in bugs if bug["severity"] in severities] if subset: severity_counts.append( - reo.bug_link( - subset, template, reo.with_severities(query, severities) - ) + bug_link(subset, template, with_severities(query, severities)) ) if severity_counts: - lines.append(reo.SUB_BULLET + ", ".join(severity_counts)) + lines.append(SUB_BULLET + ", ".join(severity_counts)) return "\n".join(lines) + def post_message(self, blocks: list[dict]) -> None: + """Post the message to Slack, or print it when the run isn't for real. + + A dry run prints what it would have posted, so `--production` means here + what it means for every other rule. `test_mode` is honoured alongside it + for the reason `triage_owner_rotations` honours it: a test run must reach + nobody. + """ + if self.dryrun or self.test_mode: + print("DRY RUN: message not posted.\n") + for block in blocks: + print(block_text(block)) + return + + # HEADING is the message's notification fallback text, which is what a + # client that cannot render blocks shows instead of them. + slack.post_to_slack(self.channel, HEADING, blocks=blocks) + logger.info("Rule %s posted to %s", self.name(), self.channel) + def get_email_data(self, date: str) -> EmailData: """Post the message, and give `send_email` nothing to send. @@ -287,9 +701,7 @@ def get_email_data(self, date: str) -> EmailData: "No data" line `send_email` then logs is about that email, not about the message, which has been posted by the time it is written. """ - reo.post_message( - self, self.channel, HEADING, self.blocks(reo.versions_to_report()) - ) + self.post_message(self.blocks(versions_to_report())) return [] @@ -325,7 +737,7 @@ def blocks(self, versions: dict[str, int]) -> list[dict]: sections.append(f"{header}\n{groups[0]}") sections.extend(groups[1:]) - return reo.to_blocks(sections) + return to_blocks(sections) if __name__ == "__main__": diff --git a/bugbot/rules/reo_regression_slack_daily.py b/bugbot/rules/reo_regression_slack_daily.py index ac7503c07..678b680a9 100644 --- a/bugbot/rules/reo_regression_slack_daily.py +++ b/bugbot/rules/reo_regression_slack_daily.py @@ -16,20 +16,103 @@ things somebody has to do, so a day skipped is a day nobody was asked. Every count links to a Bugzilla list of exactly the bugs counted. Restricted -bugs are counted and linked like any other but never named; see -`bugbot.reo_regressions`, which also holds the open regressions query and the -posting. +bugs are counted in the totals and included in the links like any other, but +never named: no message prints a bug summary, which is the same line +`BzCleaner.get_summary` draws. The top-level bullet says how many of its count +are restricted, because a reader without access opens the link and finds a +shorter list than the number they clicked on. See `restricted_note`. """ import argparse import datetime +import functools from typing import Any from libmozdata import utils as lmdutils -from bugbot import logger, utils -from bugbot import reo_regressions as reo +from bugbot import logger, slack, utils from bugbot.bzcleaner import Bug, BzCleaner, BzParams, EmailData +from bugbot.components import ComponentName, fetch_component_teams + +# Shared with the rest of bugbot rather than redeclared as ("S1", "S2") the way the +# REO queries have it. It also carries the pre-S1 names — critical maps to S1, major +# and blocker to S2 (see `constants.OLD_SEVERITY_MAP`) — so an old bug that never had +# its severity restated still lands in the S2+ counts instead of quietly missing from +# them. That is a wider net than bugdash casts, so these numbers can run slightly +# ahead of the REO tab's. +from bugbot.constants import HIGH_SEVERITY + +# The channel this rule posts to. Here rather than in `configs/rules.json` +# because it is not a secret, and because changing where an unattended recurring +# message lands should take a code review -- the same reasoning `frontend_triage` +# gives for keeping its component list in code. The bot token is the part that is +# a secret, and that stays in `configs/config.json`. +# +# TEMPORARY: this is currently #tmp-dm-test, a scratch channel for shaking the +# port out. It has to be pointed at the real REO channel before this message is +# meant for anyone to read. +CHANNEL = "C0BLP0WUBED" + +BZ_BUGLIST_URL = "https://bugzilla.mozilla.org/buglist.cgi" + +# Every Bugzilla classification except Graveyard, which holds the ~100 retired +# products. Same list bugdash's REO queries use. +# +# "Developer Infrastructure" appearing here and in EXCLUDED_PRODUCTS is not a +# contradiction: the classification also holds Firefox Build System, Conduit and +# Tree Management, which stay in scope, and only the product of the same name is +# dropped. +CLASSIFICATIONS = [ + "Client Software", + "Components", + "Developer Infrastructure", + "Other", + "Server Software", +] + +# The severity that means no triage decision has been made yet. Bugs are filtered +# on this locally, so the value has to be exactly what Bugzilla reports in a bug's +# severity field, which is case sensitive and not always what the same value looks +# like in a search: "N/A" comes back from the API where a query matches it as +# "n/a". Only "--" counts as missing here; N/A is a decision, not the absence of +# one. +MISSING_SEVERITIES = ("--",) + +# Products dropped from every query, so their bugs reach no bucket in this +# message and none of the burndown lines. +# +# Not the `exclude_products` key some rules carry in `configs/rules.json`: that one +# subtracts from `BzCleaner`'s default product list, which these classification +# scoped queries never use, so the name would mean something different here. +EXCLUDED_PRODUCTS = ("Testing", "Developer Infrastructure") + +# Where the product exclusions are numbered from in a boolean chart. Above every +# slot either query uses -- `regressions_query` and `burndown_query`. +EXCLUDED_PRODUCTS_SLOT = 12 + +# For a component with no team_name, or one missing from the mapping entirely. +# Every component had a team when this was written, so this is only a guard +# against silently dropping bugs out of the per-team line. +UNKNOWN_TEAM = "Unknown team" + +# A Slack section block holds at most 3000 characters. +SECTION_LIMIT = 3000 + +# Above this length a snapshot URL is shortened, and failing that dropped +# entirely -- see `bug_link`. Keeps one very long bug list from pushing a section +# over SECTION_LIMIT. +MAX_SNAPSHOT_URL = 2000 + +# Slack renders this back as >. Sending the character itself would work where it +# is used now, but it ends a link's label at the first > and opens a blockquote at +# the start of a line, so a label or bullet reworded around it would break in ways +# that are easy to miss. The entity is never wrong. +GREATER_THAN = ">" + +# Slack has no nested lists in message text, so indent sub-bullets by hand. +# Four plain spaces; if Slack ever collapses them, non-breaking spaces (U+00A0) +# are the fix. +SUB_BULLET = " ◦ " # Products where an unassigned high severity bug is not something to nag about, # so they are left out of the "S2+ unassigned" bucket alone. Empty today, as the @@ -113,12 +196,81 @@ # news rather than as the script having failed. NOTHING_STUCK = "• Nothing needs attention" -# What this message needs back from a bug search on top of `reo.BUG_FIELDS`: it -# ages every bug, and the three timestamps it can age one from all live on the -# bug itself, so asking for them keeps it to the same one request per version. -# Still no `summary` field -- see `reo.BUG_FIELDS`. -FIELDS = f"{reo.BUG_FIELDS},assigned_to,creation_time,last_change_time,flags" -BURNDOWN_FIELDS = f"{reo.BUG_FIELDS},cf_last_resolved" +# What a bug search has to come back with for this message: it ages every bug, +# and the three timestamps it can age one from all live on the bug itself, so +# asking for them keeps it to the same one request per version. +# +# `groups` is how a bug is known to be restricted, and the message counts those. +# See `restricted_note`. +# +# No `summary` field, in either of these. That is the line this message does not +# cross, and the same one `BzCleaner.get_summary` draws. +BUG_FIELDS = "id,severity,product,component,groups" +FIELDS = f"{BUG_FIELDS},assigned_to,creation_time,last_change_time,flags" +BURNDOWN_FIELDS = f"{BUG_FIELDS},cf_last_resolved" + + +def without_excluded_products(slot: int = EXCLUDED_PRODUCTS_SLOT) -> dict: + """Chart conditions dropping EXCLUDED_PRODUCTS, a numbered slot per product. + + One ANDed notequals per product rather than a single nowords: Bugzilla splits + a nowords value on whitespace, so "Developer Infrastructure" would be matched + as the two words separately and drop products nobody asked to exclude. + """ + conditions: dict = {} + for offset, product in enumerate(EXCLUDED_PRODUCTS): + number = slot + offset + conditions |= { + f"f{number}": "product", + f"o{number}": "notequals", + f"v{number}": product, + } + + return conditions + + +def regressions_query(version: int) -> dict: + """Build the open regressions query for a version. + + Bugs with all of the following: + - regression keyword + - open (unresolved) + - status-firefox{version} is affected + Bugs with any of the following are ignored: + - tracking-firefox{version} is - + - stalled or intermittent-failure keywords + - within one of EXCLUDED_PRODUCTS + + The cycle summary splits this set into the bugs that regressed during the + cycle and the ones that were already there; this message wants the whole set, + so it has no equivalent of that condition. + + Nothing here filters on `bug_group`: an authenticated search returns every bug + the key can see, so restricted regressions arrive on their own. + + Field numbering is Bugzilla's boolean charts: f/o/v are the field, operator + and value for a numbered condition, OP and CP open and close a group, j sets + how a group joins (OR here, AND otherwise) and n negates. The gaps at f2-f7 + and f9 are harmless, as Bugzilla ignores unused numbers: f7 comes from + bugdash, f2-f6 are where the cycle summary puts its split, and f9 is free now + the product exclusions live at EXCLUDED_PRODUCTS_SLOT. + """ + return { + "classification": CLASSIFICATIONS, + "keywords": "regression", + "keywords_type": "allwords", + "resolution": "---", + "f1": utils.get_flag(version, "status", "release"), + "o1": "equals", + "v1": "affected", + "f8": utils.get_flag(version, "tracking", "release"), + "o8": "notequals", + "v8": "-", + "f10": "keywords", + "o10": "nowordssubstr", + "v10": "stalled,intermittent-failure", + **without_excluded_products(), + } def burndown_query(version: int, uplift_flag: str) -> dict: @@ -132,13 +284,13 @@ def burndown_query(version: int, uplift_flag: str) -> dict: - in a security group - tracking-firefox{version} is +, ? or blocking Bugs with any of the following are ignored: - - within one of reo.EXCLUDED_PRODUCTS + - within one of EXCLUDED_PRODUCTS - an uplift request against the channel, in any state All but the last of those is bugdash's Burndown list, kept in step with app/buglists/burndown.mjs there. Its numbering gaps at f5, f8 and f10 are copied along with the rest, as Bugzilla ignores unused numbers, and f9 is - free now the product exclusions live at reo.EXCLUDED_PRODUCTS_SLOT. + free now the product exclusions live at EXCLUDED_PRODUCTS_SLOT. The f3-f7 group is what narrows "every fix still marked affected" down to the fixes worth chasing an uplift for, and being a security bug is one of the three @@ -157,9 +309,9 @@ def burndown_query(version: int, uplift_flag: str) -> dict: it. n11 negates that, leaving the fixes nobody has asked to uplift. """ return { - "classification": reo.CLASSIFICATIONS, + "classification": CLASSIFICATIONS, "resolution": "FIXED", - "f1": reo.status_flag(version), + "f1": utils.get_flag(version, "status", "release"), "o1": "anywords", "v1": "affected optional", "j2": "OR", @@ -170,7 +322,7 @@ def burndown_query(version: int, uplift_flag: str) -> dict: "f4": "bug_group", "o4": "substring", "v4": "sec", - "f6": reo.tracking_flag(version), + "f6": utils.get_flag(version, "tracking", "release"), "o6": "anywordssubstr", "v6": "+ ? blocking", "f7": "CP", @@ -178,10 +330,193 @@ def burndown_query(version: int, uplift_flag: str) -> dict: "o11": "substring", "v11": uplift_flag, "n11": "1", - **reo.without_excluded_products(), + **without_excluded_products(), } +def snapshot_url(bugs: list[dict]) -> str: + """A Bugzilla URL listing exactly these bugs, as bugdash's bug lists do. + + Linking the bug ids rather than the query means the list still matches the + count in the message when it is read days later. order=bug_list keeps + Bugzilla showing them in the order given rather than re-sorting. + + Built by hand rather than through `utils.get_bz_search_url` so the separators + stay as commas: percent-encoded they would triple in length, and the length is + what `MAX_SNAPSHOT_URL` is measuring. + + Restricted bugs are in here with everything else. A reader without access gets + a shorter list than the count that linked them here, which is what the + "(n restricted)" note on the bullet is for. + """ + ids = ",".join(str(bug["id"]) for bug in bugs) + + return f"{BZ_BUGLIST_URL}?bug_id={ids}&order=bug_list" + + +def shortened_url(url: str) -> str | None: + """A short Bugzilla URL for a long one, or None if it couldn't be shortened. + + `utils.shorten_long_bz_url` answers a shortener error by returning the URL + split across several lines (bugbot#1402). Harmless in an email, useless in a + Slack link, which would end at the first newline — so a multi-line answer is + treated as a failure here rather than posted. + + Any other failure is swallowed for the same reason: the count is the message + and the link is a convenience, so a shortener that is down should cost the link + and nothing more. + """ + try: + short = utils.shorten_long_bz_url(url) + except Exception: + logger.exception("Could not shorten a Bugzilla URL") + return None + + if "\n" in short or len(short) > MAX_SNAPSHOT_URL: + return None + + return short + + +def bug_link(bugs: list[dict], label_template: str) -> str: + """Format a non-empty bug list as a Slack link labelled with its count. + + label_template is formatted with the count, e.g. "{} S2+ unassigned". + + A snapshot URL that comes out too long is shortened, which keeps the link + pointing at exactly the bugs counted, and failing that the count is left + unlinked. No line here has a live query to fall back on: the ageing is done + in this rule rather than by Bugzilla, and reproducing a team as a query means + listing all its components. + + Callers are expected to skip empty lists: an empty bug_id would link to a + broken list, and a count of zero is left out of the message anyway. + """ + label = label_template.format(len(bugs)) + snapshot = snapshot_url(bugs) + + if len(snapshot) <= MAX_SNAPSHOT_URL: + return f"<{snapshot}|{label}>" + + url = shortened_url(snapshot) + if url is None: + return label + + return f"<{url}|{label}>" + + +@functools.cache +def component_teams() -> dict[ComponentName, str]: + """Map every (product, component) to the team that owns it. + + team_name is a Bugzilla field on components, the same one bugdash's Teams + filter uses. One request covers every product, around 120KB for 2000-odd + components, which is why it's cached for the life of the run. + """ + return fetch_component_teams() + + +def team_of(bug: dict) -> str: + """The team owning a bug's component.""" + return component_teams().get(ComponentName.from_bug(bug)) or UNKNOWN_TEAM + + +def team_breakdown(bugs: list[dict]) -> str: + """Count the bugs owned by each team, busiest team first. + + Every team is listed rather than just the top few, so that the line works + as a nudge to each team that owns something. + """ + by_team: dict[str, list[dict]] = {} + for bug in bugs: + by_team.setdefault(team_of(bug), []).append(bug) + + ranked = sorted(by_team.items(), key=lambda item: (-len(item[1]), item[0])) + + return ", ".join(bug_link(team_bugs, f"{{}} {team}") for team, team_bugs in ranked) + + +def restricted_note(bugs: list[dict]) -> str: + """Say how many of a bug list are restricted, or nothing when none are. + + A bug is restricted when it is in any group at all, not only a security one: + the note exists to explain why the linked list looks shorter than the count to + a reader without access, and that gap opens for an employee-confidential or + partner group just as it does for `core-security`. That is a wider test than + the `bug_group ~ "sec"` branch in `burndown_query`, which is asking a + different question — whether a fix is worth chasing, not whether it is + readable. + + Deliberately plain text rather than part of the link label, so the blue runs as + far as the thing being counted and no further, and deliberately only used on the + top-level bullets: repeated on every team sub-bullet it would say little and + crowd out the counts that are the point of those lines. + """ + count = sum(1 for bug in bugs if bug.get("groups")) + if not count: + return "" + + return f" ({count} restricted)" + + +def to_blocks(sections: list[str]) -> list[dict]: + """Wrap the sections of a message as Block Kit sections. + + Slack silently splits a message whose text runs past about 4000 characters + into several messages, which is what happened when every count linked to a + full query URL. Snapshot URLs brought the total well under that, but each + section block gets its own 3000 character allowance, so keeping the sections + means a busier day can't start splitting the message again. + + A section that does overflow raises rather than posting something malformed. + The team breakdown is the part that could get there, at roughly 90 characters + per team; capping or splitting it is the fix if that ever fires. + """ + for section in sections: + if len(section) > SECTION_LIMIT: + raise RuntimeError( + f"Slack section block is {len(section)} characters, over the " + f"{SECTION_LIMIT} limit:\n{section[:200]}..." + ) + + return [ + {"type": "section", "text": {"type": "mrkdwn", "text": section}} + for section in sections + ] + + +def block_text(block: dict) -> str: + """The text of any block, for printing a message instead of posting it. + + Section and header blocks keep their text in one place and context blocks in + a list of elements, so a dry run has to handle both rather than assume the + shape of the blocks it was handed. + """ + if "elements" in block: + return " ".join(element["text"] for element in block["elements"]) + + return block["text"]["text"] + + +def versions_to_report() -> dict[str, int]: + """The current version of each channel, with what was read written to the log. + + From the trains API rather than through `BzCleaner.init_versions`: + `utils.get_checked_versions` returns nothing on merge day, and + `has_enough_data` would then skip the run on exactly the day this message + has its own wording for. + """ + versions = utils.get_versions_from_trains() + logger.info( + "Reporting Firefox %s release / %s beta / %s nightly", + versions["release"], + versions["beta"], + versions["nightly"], + ) + + return versions + + def stuck_since() -> datetime.datetime: """The moment a bug has to predate to count as stuck. See STUCK_HOURS.""" now = datetime.datetime.now(datetime.timezone.utc) @@ -215,7 +550,7 @@ def needs_assignee(bug: dict, cutoff: datetime.datetime) -> bool: overlooked. See unassigned_exempt(). """ return ( - bug["severity"] in reo.HIGH_SEVERITY + bug["severity"] in HIGH_SEVERITY and not unassigned_exempt(bug) and utils.is_no_assignee(bug["assigned_to"]) and lmdutils.get_date_ymd(bug["creation_time"]) < cutoff @@ -231,7 +566,7 @@ def needs_severity(bug: dict, cutoff: datetime.datetime) -> bool: team asking questions or adjusting metadata. """ return ( - bug["severity"] in reo.MISSING_SEVERITIES + bug["severity"] in MISSING_SEVERITIES and lmdutils.get_date_ymd(bug["last_change_time"]) < cutoff ) @@ -280,18 +615,15 @@ def stuck_group(bugs: list[dict], label: str, anchor: str) -> str: Empty buckets return an empty string and are left out of the message, so it stays a list of things to do rather than a scoreboard of zeros. - - Neither link gets a fallback query: the ageing is done here rather than by - Bugzilla, so there is no query URL that reproduces either count. """ if not bugs: return "" - age = f", {reo.GREATER_THAN} {STUCK_HOURS} hours since {anchor}" + age = f", {GREATER_THAN} {STUCK_HOURS} hours since {anchor}" return ( - f"• {reo.bug_link(bugs, label)}{reo.restricted_note(bugs)}{age}\n" - f"{reo.SUB_BULLET}{reo.team_breakdown(bugs)}" + f"• {bug_link(bugs, label)}{restricted_note(bugs)}{age}\n" + f"{SUB_BULLET}{team_breakdown(bugs)}" ) @@ -309,7 +641,7 @@ class ReoRegressionSlackDaily(BzCleaner): # Where the message goes. A `--channel` run overrides it, so this is the # channel the cron posts to; see `parse_custom_arguments`. - channel = reo.CHANNEL + channel = CHANNEL def description(self) -> str: return "REO release regressions needing action posted to Slack" @@ -317,7 +649,7 @@ def description(self) -> str: def all_include_fields(self) -> bool: # The fields a search asks for are FIELDS and BURNDOWN_FIELDS and # nothing else. `BzCleaner` would otherwise add `summary` to every - # query, which is the one field no message here prints. + # query, which is the one field this message does not print. return True def has_default_products(self) -> bool: @@ -333,10 +665,18 @@ def filter_no_nag_keyword(self) -> bool: return False def add_custom_arguments(self, parser: argparse.ArgumentParser) -> None: - reo.add_channel_argument(parser) + parser.add_argument( + "--channel", + action="store", + default="", + help=( + f"Slack channel ID to post to, overriding {CHANNEL}. Useful to " + "shake the message out somewhere else without editing the code." + ), + ) def parse_custom_arguments(self, args: argparse.Namespace) -> None: - self.channel = args.channel or reo.CHANNEL + self.channel = args.channel or CHANNEL def get_bz_params(self, date: str) -> BzParams: """The query the running `get_bugs()` call is for. See `fetch_bugs`.""" @@ -379,7 +719,7 @@ def open_regressions(self, versions: dict[str, int]) -> list[dict]: """ bugs: dict[int, dict] = {} for version in sorted(set(versions.values())): - for bug in self.fetch_bugs(reo.regressions_query(version)): + for bug in self.fetch_bugs(regressions_query(version)): bugs[bug["id"]] = bug return list(bugs.values()) @@ -408,6 +748,25 @@ def burndown_group( return stuck_group(bugs, label, "resolved") + def post_message(self, blocks: list[dict]) -> None: + """Post the message to Slack, or print it when the run isn't for real. + + A dry run prints what it would have posted, so `--production` means here + what it means for every other rule. `test_mode` is honoured alongside it + for the reason `triage_owner_rotations` honours it: a test run must reach + nobody. + """ + if self.dryrun or self.test_mode: + print("DRY RUN: message not posted.\n") + for block in blocks: + print(block_text(block)) + return + + # HEADING is the message's notification fallback text, which is what a + # client that cannot render blocks shows instead of them. + slack.post_to_slack(self.channel, HEADING, blocks=blocks) + logger.info("Rule %s posted to %s", self.name(), self.channel) + def get_email_data(self, date: str) -> EmailData: """Post the message, and give `send_email` nothing to send. @@ -417,9 +776,7 @@ def get_email_data(self, date: str) -> EmailData: "No data" line `send_email` then logs is about that email, not about the message, which has been posted by the time it is written. """ - reo.post_message( - self, self.channel, HEADING, self.blocks(reo.versions_to_report()) - ) + self.post_message(self.blocks(versions_to_report())) return [] @@ -471,7 +828,7 @@ def blocks(self, versions: dict[str, int]) -> list[dict]: sections.extend(groups or [NOTHING_STUCK]) - return titles + reo.to_blocks(sections) + return titles + to_blocks(sections) if __name__ == "__main__": diff --git a/tests/rules/test_reo_regression_slack.py b/tests/rules/test_reo_regression_slack.py index e02f7b206..10a88a135 100644 --- a/tests/rules/test_reo_regression_slack.py +++ b/tests/rules/test_reo_regression_slack.py @@ -4,8 +4,8 @@ import datetime -from bugbot import reo_regressions as reo from bugbot import utils +from bugbot.rules import reo_regression_slack as summary from bugbot.rules.reo_regression_slack import ReoRegressionSlack @@ -50,7 +50,7 @@ def test_regression_group_notes_restricted_bugs_on_the_top_line_only(monkeypatch ] rule = ReoRegressionSlack() monkeypatch.setattr(rule, "fetch_bugs", lambda query, fields=None: bugs) - monkeypatch.setattr(reo, "team_of", lambda bug: "Team A") + monkeypatch.setattr(summary, "team_of", lambda bug: "Team A") bullet, teams, severities = rule.regression_group( 150, False, "New", by_team=True @@ -69,8 +69,8 @@ def test_the_channel_is_a_constant_the_flag_can_override(): # No flag means the module constant, which is what the cron posts to. rule.parse_custom_arguments(parser.parse_args([])) - assert rule.channel == reo.CHANNEL - assert reo.CHANNEL.startswith("C") + assert rule.channel == summary.CHANNEL + assert summary.CHANNEL.startswith("C") rule.parse_custom_arguments(parser.parse_args(["--channel", "C_TEST"])) assert rule.channel == "C_TEST" diff --git a/tests/rules/test_reo_regression_slack_daily.py b/tests/rules/test_reo_regression_slack_daily.py index 8260ca97f..3b8acca27 100644 --- a/tests/rules/test_reo_regression_slack_daily.py +++ b/tests/rules/test_reo_regression_slack_daily.py @@ -4,7 +4,6 @@ import datetime -from bugbot import reo_regressions as reo from bugbot import utils from bugbot.rules import reo_regression_slack_daily as daily from bugbot.rules.reo_regression_slack_daily import ReoRegressionSlackDaily @@ -30,13 +29,13 @@ def test_the_burndown_query_drops_the_excluded_products(): excluded = { query[f"v{n}"] for n in range( - reo.EXCLUDED_PRODUCTS_SLOT, - reo.EXCLUDED_PRODUCTS_SLOT + len(reo.EXCLUDED_PRODUCTS), + daily.EXCLUDED_PRODUCTS_SLOT, + daily.EXCLUDED_PRODUCTS_SLOT + len(daily.EXCLUDED_PRODUCTS), ) if query.get(f"f{n}") == "product" } - assert excluded == set(reo.EXCLUDED_PRODUCTS) + assert excluded == set(daily.EXCLUDED_PRODUCTS) def test_uplift_flags_come_from_get_flag(): @@ -69,7 +68,7 @@ def test_every_search_asks_for_the_groups_field(): def test_no_search_asks_for_a_bug_summary(): - # Extending `reo.BUG_FIELDS` must not be what quietly starts naming restricted + # Extending `daily.BUG_FIELDS` must not be what quietly starts naming restricted # bugs. for fields in (daily.FIELDS, daily.BURNDOWN_FIELDS): assert "summary" not in fields.split(",") @@ -85,7 +84,7 @@ def test_the_ageing_fields_are_asked_for(): def test_stuck_group_puts_the_note_outside_the_link_and_before_the_age(monkeypatch): - monkeypatch.setattr(reo, "team_of", lambda bug: "Team A") + monkeypatch.setattr(daily, "team_of", lambda bug: "Team A") bugs = [ {"id": 1, "groups": ["core-security-release"]}, {"id": 2, "groups": []}, diff --git a/tests/test_reo_regressions.py b/tests/test_reo_regressions.py deleted file mode 100644 index 801f81022..000000000 --- a/tests/test_reo_regressions.py +++ /dev/null @@ -1,293 +0,0 @@ -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this file, -# You can obtain one at http://mozilla.org/MPL/2.0/. - -import pytest - -from bugbot import constants, utils -from bugbot import reo_regressions as reo -from bugbot.bzcleaner import BzCleaner -from bugbot.rules.reo_regression_slack import ReoRegressionSlack -from bugbot.rules.reo_regression_slack_daily import ReoRegressionSlackDaily - -# Both messages, for the behaviour they share as `BzCleaner` rules. What one of -# them does on its own is in its own test file. -RULES = (ReoRegressionSlack, ReoRegressionSlackDaily) - - -def test_regressions_query_without_a_split_asks_for_the_whole_set(): - query = reo.regressions_query(150) - - assert query["f1"] == "cf_status_firefox150" - assert query["v1"] == "affected" - # No condition on the previous version, so neither side of the split. - assert "f2" not in query - assert "n2" not in query - - -def test_regressions_query_new_looks_at_the_previous_version(): - query = reo.regressions_query(150, carry_over=False) - - assert query["j2"] == "OR" - assert query["f3"] == query["f4"] == query["f5"] == "cf_status_firefox149" - assert {query["v3"], query["v4"], query["v5"]} == {"unaffected", "?", "---"} - assert "n2" not in query - - -def test_regressions_query_carry_over_negates_the_whole_group(): - new = reo.regressions_query(150, carry_over=False) - carry_over = reo.regressions_query(150, carry_over=True) - - # The two partition the set, so the only difference between them is the - # negation, and it has to sit on the OP at f2 rather than on the first - # condition inside the group. - assert carry_over == {**new, "n2": "1"} - assert carry_over["f2"] == "OP" - - -@pytest.mark.parametrize("carry_over", (None, False, True)) -def test_the_regressions_query_drops_the_excluded_products(carry_over): - # The burndown query is the other one that has to, and its own test covers it. - query = reo.regressions_query(150, carry_over) - excluded = { - query[f"v{n}"] - for n in range( - reo.EXCLUDED_PRODUCTS_SLOT, - reo.EXCLUDED_PRODUCTS_SLOT + len(reo.EXCLUDED_PRODUCTS), - ) - if query.get(f"f{n}") == "product" - } - - assert excluded == set(reo.EXCLUDED_PRODUCTS) - - -def test_each_excluded_product_gets_its_own_condition(): - # Not one nowords: Bugzilla splits that value on whitespace, so - # "Developer Infrastructure" would match as two separate words and drop - # products nobody asked to exclude. - conditions = reo.without_excluded_products() - - assert len(conditions) == 3 * len(reo.EXCLUDED_PRODUCTS) - assert all( - op == "notequals" for key, op in conditions.items() if key.startswith("o") - ) - assert any( - " " in v for k, v in conditions.items() if k.startswith("v") - ), "the multi-word product is what makes the per-product split necessary" - - -def test_the_exclusion_slots_clear_every_other_slot_in_use(): - # with_severities takes 11, and the burndown's uplift flag takes 11 too, so - # the exclusions have to start above both. - query = reo.with_severities(reo.regressions_query(150), ("S1", "S2")) - used = {int(k[1:]) for k in query if k[0] in "fov" and k[1:].isdigit()} - assert reo.EXCLUDED_PRODUCTS_SLOT > max( - n for n in used if n < reo.EXCLUDED_PRODUCTS_SLOT - ) - - -def test_flag_names_come_from_get_flag(): - # utils.get_flag is the one place version numbers become flag names; these - # queries must not concatenate their own. - query = reo.regressions_query(150, carry_over=False) - - assert query["f1"] == utils.get_flag(150, "status", "release") - assert query["f8"] == utils.get_flag(150, "tracking", "release") - assert query["f3"] == utils.get_flag(149, "status", "release") - - -def test_the_shared_fields_ask_for_the_groups_field(): - # Without it no message can tell a restricted bug from a public one, and both - # rules build their field lists out of this one. - assert "groups" in reo.BUG_FIELDS.split(",") - - -def test_the_shared_fields_never_ask_for_a_bug_summary(): - # A restricted bug is counted and linked, never named, and not asking for the - # field is what makes that true of anything built on this list. - assert "summary" not in reo.BUG_FIELDS.split(",") - - -def test_restricted_note_counts_bugs_in_any_group(): - bugs = [ - {"id": 1, "groups": ["core-security-release"]}, - {"id": 2, "groups": ["mozilla-employee-confidential"]}, - {"id": 3, "groups": []}, - ] - - # Wider than the `bug_group ~ "sec"` branch in the daily rule's burndown query - # on purpose: the note explains why the linked list looks short, and any group - # does that. - assert reo.restricted_note(bugs) == " (2 restricted)" - - -@pytest.mark.parametrize("bugs", ([], [{"id": 1, "groups": []}])) -def test_restricted_note_is_silent_when_nothing_is_restricted(bugs): - assert reo.restricted_note(bugs) == "" - - -LONG_LIST = [{"id": 1000000 + i} for i in range(300)] -SHORT_URL = "https://bugzilla.mozilla.org/1a2b3c" - - -def test_bug_link_shortens_a_snapshot_that_is_too_long(monkeypatch): - # The shortener is preferred over the fallback query: it still points at - # exactly the bugs counted, where a live query can drift from the count. - monkeypatch.setattr(reo.utils, "shorten_long_bz_url", lambda url: SHORT_URL) - - link = reo.bug_link(LONG_LIST, "{} New Regressions", {"resolution": "---"}) - - assert link == f"<{SHORT_URL}|300 New Regressions>" - - -def test_bug_link_shortens_a_team_line_that_has_no_fallback(monkeypatch): - # Team lines pass no fallback, as reproducing a team as a query means listing - # all of its components, so before the shortener they lost their link entirely. - monkeypatch.setattr(reo.utils, "shorten_long_bz_url", lambda url: SHORT_URL) - - assert reo.bug_link(LONG_LIST, "{} Media") == f"<{SHORT_URL}|300 Media>" - - -def test_bug_link_rejects_the_multiline_shortener_fallback(monkeypatch): - # utils.shorten_long_bz_url answers a shortener error with the URL split over - # lines (bugbot#1402). A Slack link would end at the first newline, so that has - # to count as a failure and drop through to the query. - monkeypatch.setattr( - reo.utils, "shorten_long_bz_url", lambda url: "https://a\nhttps://b" - ) - - link = reo.bug_link(LONG_LIST, "{} New Regressions", {"resolution": "---"}) - - assert "buglist.cgi?resolution=---" in link - assert "\n" not in link - - -def test_bug_link_survives_a_shortener_that_raises(monkeypatch): - def boom(url): - raise RuntimeError("shortener down") - - monkeypatch.setattr(reo.utils, "shorten_long_bz_url", boom) - - link = reo.bug_link(LONG_LIST, "{} New Regressions", {"resolution": "---"}) - - assert "buglist.cgi?resolution=---" in link - - -def test_bug_link_is_left_unlinked_when_nothing_works(monkeypatch): - monkeypatch.setattr( - reo.utils, "shorten_long_bz_url", lambda url: "https://a\nhttps://b" - ) - - assert reo.bug_link(LONG_LIST, "{} Media") == "300 Media" - - -def test_bug_link_does_not_shorten_a_url_that_fits(monkeypatch): - def unexpected(url): - raise AssertionError("a short URL should never reach the shortener") - - monkeypatch.setattr(reo.utils, "shorten_long_bz_url", unexpected) - - assert reo.bug_link([{"id": 1}], "{} Media") == ( - "" - ) - - -def test_to_blocks_wraps_each_section_on_its_own(): - assert reo.to_blocks(["one", "two"]) == [ - {"type": "section", "text": {"type": "mrkdwn", "text": "one"}}, - {"type": "section", "text": {"type": "mrkdwn", "text": "two"}}, - ] - - -def test_to_blocks_refuses_to_post_an_overflowing_section(): - with pytest.raises(RuntimeError, match="over the 3000 limit"): - reo.to_blocks(["x" * (reo.SECTION_LIMIT + 1)]) - - -def test_block_text_reads_every_block_shape(): - blocks = [ - {"type": "header", "text": {"type": "plain_text", "text": "Title"}}, - {"type": "context", "elements": [{"type": "mrkdwn", "text": "Daily update"}]}, - {"type": "section", "text": {"type": "mrkdwn", "text": "Body"}}, - ] - - assert [reo.block_text(block) for block in blocks] == [ - "Title", - "Daily update", - "Body", - ] - - -@pytest.mark.parametrize("rule_class", RULES) -def test_both_messages_are_bzcleaner_rules(rule_class): - # The searches, the arguments, the must_run gate and the error handling are - # the framework's; what these two add is where the report goes. - assert isinstance(rule_class(), BzCleaner) - - -@pytest.mark.parametrize("rule_class", RULES) -def test_a_query_goes_out_as_the_rule_built_it(rule_class): - rule = rule_class() - params = {**reo.regressions_query(150), "include_fields": reo.BUG_FIELDS} - rule.amend_bzparams(params, []) - - # No `summary`: a restricted bug is counted and linked, never named. - assert params["include_fields"] == reo.BUG_FIELDS - # No default product list, no [no-nag] exclusion and no group filter. The - # query is bugdash's, and what it matches is what gets counted. - assert "product" not in params - assert "[no-nag]" not in params.values() - assert "bug_group" not in params.values() - - -@pytest.mark.parametrize("rule_class", RULES) -def test_neither_message_caches_the_bugs_it_reports(rule_class): - # A bug belongs in these messages until somebody acts on it, so the cache - # that keeps other rules from repeating themselves has to stay off. It is by - # default -- `max_days_in_cache` is -1 -- and this is what would catch a - # configs/rules.json entry turning it on. - rule = rule_class() - rule.cache.set_dry_run(False) # as a --production run does - - assert rule.max_days_in_cache() < 1 - assert 1234 not in rule.cache - - -@pytest.mark.parametrize("rule_class", RULES) -def test_the_report_is_a_slack_message_rather_than_an_email(rule_class, monkeypatch): - posted = [] - rule = rule_class() - rule.dryrun = False - rule.test_mode = False - monkeypatch.setattr(reo, "versions_to_report", lambda: {}) - monkeypatch.setattr(rule, "blocks", lambda versions: ["a block"]) - monkeypatch.setattr( - reo.slack, - "post_to_slack", - lambda channel, text, blocks=None: posted.append((channel, text, blocks)) - or "1.0", - ) - - # The empty list is what stops `send_email` sending anything. - assert rule.get_email_data("today") == [] - - (channel, text, blocks), *rest = posted - assert not rest - assert (channel, blocks) == (reo.CHANNEL, ["a block"]) - assert text, "the message needs its notification fallback text" - - -def test_high_severity_is_the_shared_constant(): - # Shared with the rest of bugbot rather than the REO queries' ("S1", "S2"), so - # the legacy names count too. - assert reo.HIGH_SEVERITY is constants.HIGH_SEVERITY - assert {"S1", "S2", "critical", "major", "blocker"} <= reo.HIGH_SEVERITY - - -def test_with_severities_builds_a_stable_url(): - # HIGH_SEVERITY is a set, and set iteration order is not stable across - # processes, so the value has to be sorted or the URL changes run to run. - query = reo.with_severities({"resolution": "---"}, reo.HIGH_SEVERITY) - - assert query["v11"] == ", ".join(sorted(reo.HIGH_SEVERITY)) - assert query["o11"] == "anyexact" From 6ebc64f04c38fe193e29a11efda2a309c997b131 Mon Sep 17 00:00:00 2001 From: DonalMe <76789979+DonalMe@users.noreply.github.com> Date: Wed, 16 Sep 2026 10:38:20 -0400 Subject: [PATCH 5/6] Remove unnessecary reo_regression slack tests --- tests/rules/test_reo_regression_slack.py | 76 --------- .../rules/test_reo_regression_slack_daily.py | 145 ------------------ tests/test_slack.py | 131 ---------------- 3 files changed, 352 deletions(-) delete mode 100644 tests/rules/test_reo_regression_slack.py delete mode 100644 tests/rules/test_reo_regression_slack_daily.py delete mode 100644 tests/test_slack.py diff --git a/tests/rules/test_reo_regression_slack.py b/tests/rules/test_reo_regression_slack.py deleted file mode 100644 index 10a88a135..000000000 --- a/tests/rules/test_reo_regression_slack.py +++ /dev/null @@ -1,76 +0,0 @@ -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this file, -# You can obtain one at http://mozilla.org/MPL/2.0/. - -import datetime - -from bugbot import utils -from bugbot.rules import reo_regression_slack as summary -from bugbot.rules.reo_regression_slack import ReoRegressionSlack - - -def test_the_rule_is_named_after_its_module(): - # Which is what `logger_extra["bugbot_rule"]` is tagged with, so a failure in - # this message is told apart from one in the daily message. - assert ReoRegressionSlack().name() == "reo_regression_slack" - - -def test_the_summary_runs_on_monday_and_thursday_only(): - rule = ReoRegressionSlack() - monday = datetime.datetime(2026, 8, 31) - week = [monday + datetime.timedelta(days=day) for day in range(7)] - - assert [day for day in week if rule.must_run(day)] == [ - monday, - datetime.datetime(2026, 9, 3), # Thursday - ] - - -def test_the_cadence_comes_from_the_rule_config(): - # `BzCleaner.must_run` reads it, so the days are configuration rather than an - # override here. Twice a week rather than daily: the counts move slowly, and a - # summary that arrives every morning stops being read. - assert utils.get_config("reo_regression_slack", "must_run") == ["Mon", "Thu"] - - -def test_a_run_can_be_pointed_at_another_day(): - # `BzCleaner`'s own --date, which is how the Mon/Thu gate is exercised without - # waiting for a Monday. - rule = ReoRegressionSlack() - args = rule.get_args_parser().parse_args(["-D", "2026-09-01"]) - - assert not rule.must_run(datetime.datetime(2026, 9, 1)) # Tuesday - assert args.date == "2026-09-01" - - -def test_regression_group_notes_restricted_bugs_on_the_top_line_only(monkeypatch): - bugs = [ - {"id": 1, "severity": "S2", "groups": ["core-security-release"]}, - {"id": 2, "severity": "--", "groups": []}, - ] - rule = ReoRegressionSlack() - monkeypatch.setattr(rule, "fetch_bugs", lambda query, fields=None: bugs) - monkeypatch.setattr(summary, "team_of", lambda bug: "Team A") - - bullet, teams, severities = rule.regression_group( - 150, False, "New", by_team=True - ).split("\n") - - assert bullet.endswith("|2 New Regressions> (1 restricted)") - assert "restricted" not in teams - assert "restricted" not in severities - assert "1 S2+" in severities - assert "1 missing severity" in severities - - -def test_the_channel_is_a_constant_the_flag_can_override(): - rule = ReoRegressionSlack() - parser = rule.get_args_parser() - - # No flag means the module constant, which is what the cron posts to. - rule.parse_custom_arguments(parser.parse_args([])) - assert rule.channel == summary.CHANNEL - assert summary.CHANNEL.startswith("C") - - rule.parse_custom_arguments(parser.parse_args(["--channel", "C_TEST"])) - assert rule.channel == "C_TEST" diff --git a/tests/rules/test_reo_regression_slack_daily.py b/tests/rules/test_reo_regression_slack_daily.py deleted file mode 100644 index 3b8acca27..000000000 --- a/tests/rules/test_reo_regression_slack_daily.py +++ /dev/null @@ -1,145 +0,0 @@ -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this file, -# You can obtain one at http://mozilla.org/MPL/2.0/. - -import datetime - -from bugbot import utils -from bugbot.rules import reo_regression_slack_daily as daily -from bugbot.rules.reo_regression_slack_daily import ReoRegressionSlackDaily - - -def test_the_rule_is_named_after_its_module(): - assert ReoRegressionSlackDaily().name() == "reo_regression_slack_daily" - - -def test_the_daily_message_is_ungated(): - # Every weekday the cron script invokes it: these are things somebody has to - # do, so a day skipped is a day nobody was asked. The summary is the one with - # a `must_run` in configs/rules.json. - rule = ReoRegressionSlackDaily() - monday = datetime.datetime(2026, 8, 31) - - assert utils.get_config("reo_regression_slack_daily", "must_run") is None - assert all(rule.must_run(monday + datetime.timedelta(days=day)) for day in range(7)) - - -def test_the_burndown_query_drops_the_excluded_products(): - query = daily.burndown_query(150, "approval-mozilla-beta") - excluded = { - query[f"v{n}"] - for n in range( - daily.EXCLUDED_PRODUCTS_SLOT, - daily.EXCLUDED_PRODUCTS_SLOT + len(daily.EXCLUDED_PRODUCTS), - ) - if query.get(f"f{n}") == "product" - } - - assert excluded == set(daily.EXCLUDED_PRODUCTS) - - -def test_uplift_flags_come_from_get_flag(): - for channel in daily.UPLIFT_CHANNELS: - query = daily.burndown_query(150, utils.get_flag(None, "approval", channel)) - assert query["v11"] == f"approval-mozilla-{channel}" - - -def test_burndown_query_qualifies_security_bugs(): - query = daily.burndown_query(150, "approval-mozilla-beta") - - # The f2-f7 OR group narrows "still marked affected" down to "worth chasing an - # uplift for", and being a security bug is one of the three ways in. It is not - # an access filter: it only started matching anything once bugbot's key made - # those bugs visible in the first place. - assert query["j2"] == "OR" - assert (query["f4"], query["o4"], query["v4"]) == ("bug_group", "substring", "sec") - assert (query["f2"], query["f7"]) == ("OP", "CP") - - # The uplift request is negated, so what is left is the fixes nobody has asked - # to uplift. - assert query["v11"] == "approval-mozilla-beta" - assert query["n11"] == "1" - - -def test_every_search_asks_for_the_groups_field(): - # Without it no message can tell a restricted bug from a public one. - for fields in (daily.FIELDS, daily.BURNDOWN_FIELDS): - assert "groups" in fields.split(",") - - -def test_no_search_asks_for_a_bug_summary(): - # Extending `daily.BUG_FIELDS` must not be what quietly starts naming restricted - # bugs. - for fields in (daily.FIELDS, daily.BURNDOWN_FIELDS): - assert "summary" not in fields.split(",") - - -def test_the_ageing_fields_are_asked_for(): - # Each bucket ages a bug from a different timestamp, and all of them come back - # on the bug itself, which is what keeps this to one request per version. - assert {"creation_time", "last_change_time", "flags"} <= set( - daily.FIELDS.split(",") - ) - assert "cf_last_resolved" in daily.BURNDOWN_FIELDS.split(",") - - -def test_stuck_group_puts_the_note_outside_the_link_and_before_the_age(monkeypatch): - monkeypatch.setattr(daily, "team_of", lambda bug: "Team A") - bugs = [ - {"id": 1, "groups": ["core-security-release"]}, - {"id": 2, "groups": []}, - ] - - bullet, sub_bullet = daily.stuck_group(bugs, "{} S2+ unassigned", "filed").split( - "\n" - ) - - assert bullet == ( - "• (1 restricted), > 24 hours since filed" - ) - # The note belongs to the top-level bullet alone; repeated on the team line it - # would crowd out the counts that line exists for. - assert "restricted" not in sub_bullet - - -def test_stuck_group_leaves_out_an_empty_bucket(): - assert daily.stuck_group([], "{} S2+ unassigned", "filed") == "" - - -def test_bucket_predicates_read_bugzillas_trailing_z(): - # Bugzilla stamps its timestamps with a Z, which `datetime.fromisoformat` - # only learned to read in 3.11 while bugbot still supports 3.10. Ageing goes - # through libmozdata, which has always read it, and this pins that. - cutoff = datetime.datetime(2026, 8, 17, tzinfo=datetime.timezone.utc) - bug = { - "severity": "S2", - "product": "Core", - "component": "Layout", - "assigned_to": "nobody@mozilla.org", - "creation_time": "2026-08-16T23:40:15Z", - } - - assert daily.needs_assignee(bug, cutoff) - assert not daily.needs_assignee( - {**bug, "creation_time": "2026-08-18T00:00:00Z"}, cutoff - ) - - -def test_unassigned_uses_the_shared_helper(): - # utils.is_no_assignee, so a component's `.bugs` default counts as unassigned - # the way it does everywhere else in bugbot. - cutoff = datetime.datetime(2026, 8, 17, tzinfo=datetime.timezone.utc) - bug = { - "severity": "S2", - "product": "Core", - "component": "Layout", - "creation_time": "2026-08-16T23:40:15Z", - } - - assert daily.needs_assignee({**bug, "assigned_to": "nobody@mozilla.org"}, cutoff) - assert daily.needs_assignee({**bug, "assigned_to": "gfx-bugs@mozilla.bugs"}, cutoff) - assert daily.needs_assignee({**bug, "assigned_to": ""}, cutoff) - assert not daily.needs_assignee( - {**bug, "assigned_to": "someone@mozilla.com"}, cutoff - ) diff --git a/tests/test_slack.py b/tests/test_slack.py deleted file mode 100644 index aa7605754..000000000 --- a/tests/test_slack.py +++ /dev/null @@ -1,131 +0,0 @@ -# This Source Code Form is subject to the terms of the Mozilla Public -# License, v. 2.0. If a copy of the MPL was not distributed with this file, -# You can obtain one at http://mozilla.org/MPL/2.0/. - -import json - -import pytest -import responses - -from bugbot import slack - -CHANNEL = "C0123456789" -TEST_API_URL = "https://slack.test/api/" -POST_MESSAGE_URL = f"{TEST_API_URL}chat.postMessage" - - -@pytest.fixture -def api(monkeypatch): - """Point the transport at a test server and give it a token to use.""" - monkeypatch.setenv(slack.API_URL_VAR, TEST_API_URL) - monkeypatch.setenv(slack.TOKEN_VAR, "xoxb-test") - - -@responses.activate -def test_posts_the_channel_text_and_token(api): - responses.add(responses.POST, POST_MESSAGE_URL, json={"ok": True, "ts": "1.2"}) - - assert slack.post_to_slack(CHANNEL, "a title") == "1.2" - - request = responses.calls[0].request - assert request.headers["Authorization"] == "Bearer xoxb-test" - payload = json.loads(request.body) - assert payload["channel"] == CHANNEL - assert payload["text"] == "a title" - # Unfurls would repeat what a link-heavy message already says. - assert payload["unfurl_links"] is False - assert payload["unfurl_media"] is False - # Posted under a readable name rather than the Slack app's own. - assert payload["username"] == slack.USERNAME - # Neither was passed, so neither should be sent. - assert "blocks" not in payload - assert "thread_ts" not in payload - - -@responses.activate -def test_posts_blocks_and_replies_in_thread(api): - responses.add(responses.POST, POST_MESSAGE_URL, json={"ok": True, "ts": "3.4"}) - - slack.post_to_slack( - CHANNEL, - "a title", - blocks=[{"type": "divider"}], - thread_ts="1.2", - ) - - payload = json.loads(responses.calls[0].request.body) - assert payload["blocks"] == [{"type": "divider"}] - assert payload["thread_ts"] == "1.2" - - -@responses.activate -def test_an_application_error_comes_back_as_http_200(api): - # Slack reports a rejected payload with ok=false and a 200, so the status - # alone would read as success. - responses.add( - responses.POST, POST_MESSAGE_URL, json={"ok": False, "error": "invalid_blocks"} - ) - - with pytest.raises(RuntimeError, match="invalid_blocks"): - slack.post_to_slack(CHANNEL, "a title") - - -@responses.activate -def test_missing_scope_names_the_scope_it_wanted(api): - responses.add( - responses.POST, - POST_MESSAGE_URL, - json={ - "ok": False, - "error": "missing_scope", - "needed": "chat:write", - "provided": "im:read", - }, - ) - - with pytest.raises(RuntimeError, match=r"missing_scope \(needed chat:write"): - slack.post_to_slack(CHANNEL, "a title") - - -@responses.activate -def test_an_http_error_reports_the_body(api): - responses.add(responses.POST, POST_MESSAGE_URL, body="nope", status=500) - - with pytest.raises(RuntimeError, match="HTTP 500: nope"): - slack.post_to_slack(CHANNEL, "a title") - - -def test_the_environment_token_wins_over_the_configured_one(monkeypatch): - monkeypatch.setenv(slack.TOKEN_VAR, "xoxb-env") - monkeypatch.setattr( - slack.utils, "get_login_info", lambda: {"slack_bot_token": "xoxb-cfg"} - ) - - assert slack.get_token() == "xoxb-env" - - monkeypatch.delenv(slack.TOKEN_VAR) - assert slack.get_token() == "xoxb-cfg" - - -def test_a_missing_token_stops_the_run(monkeypatch): - monkeypatch.delenv(slack.TOKEN_VAR, raising=False) - monkeypatch.setattr(slack.utils, "get_login_info", lambda: {}) - - with pytest.raises(RuntimeError, match="chat:write"): - slack.get_token() - - -def test_a_missing_config_file_is_a_missing_token_not_a_crash(monkeypatch): - def no_file(): - raise FileNotFoundError("configs/config.json") - - monkeypatch.delenv(slack.TOKEN_VAR, raising=False) - monkeypatch.setattr(slack.utils, "get_login_info", no_file) - - with pytest.raises(RuntimeError, match="chat:write"): - slack.get_token() - - -def test_the_username_is_spelled_correctly(): - # It is on every message anyone reads, so a typo here is very visible. - assert slack.USERNAME == "Firefox Release Management Bot" From 3c5a9fbad2166af2d82a6a8844aa77f02d3f5b4f Mon Sep 17 00:00:00 2001 From: DonalMe <76789979+DonalMe@users.noreply.github.com> Date: Wed, 16 Sep 2026 12:13:29 -0400 Subject: [PATCH 6/6] Clean up verbose code comments --- bugbot/rules/reo_regression_slack.py | 328 ++++---------- bugbot/rules/reo_regression_slack_daily.py | 473 +++++---------------- bugbot/slack.py | 85 +--- 3 files changed, 212 insertions(+), 674 deletions(-) diff --git a/bugbot/rules/reo_regression_slack.py b/bugbot/rules/reo_regression_slack.py index a8294a7ac..56ea3ced0 100644 --- a/bugbot/rules/reo_regression_slack.py +++ b/bugbot/rules/reo_regression_slack.py @@ -4,24 +4,11 @@ """Post the REO release regression cycle summary to Slack. -For Release, Beta and Nightly it reports two bug lists, built from the bug set -behind the REO tab of https://bugdash.moz.tools/: - -- "new regressions" carry the regression keyword and are affected in version N - while N-1 is unaffected or unknown, so they regressed during this cycle -- "carry over regressions" are the same query negated: N-1 has a real status, so - the bug was already there - -Those two partition every open regression affecting N. Each count is broken down -by severity, with New Regressions also broken down by owning team. Beta and -Nightly get a working day countdown to the end of their cycle. - -Every count links to a Bugzilla list of exactly the bugs counted. Restricted -bugs are counted in the totals and included in the links like any other, but -never named: no message prints a bug summary, which is the same line -`BzCleaner.get_summary` draws. The top-level bullet says how many of its count -are restricted, because a reader without access opens the link and finds a -shorter list than the number they clicked on. See `restricted_note`. +For Release, Beta and Nightly it reports two bug lists that together partition +every open regression affecting the version: "new regressions", affected in N +while N-1 is unaffected or unknown, and "carry over regressions", the same query +negated. Each count is broken down by severity, New Regressions also by owning +team, and Beta and Nightly get a working day countdown to the end of the cycle. The regressions that need chasing rather than counting are the other rule, `reo_regression_slack_daily`. @@ -42,23 +29,12 @@ from bugbot.bzcleaner import Bug, BzCleaner, BzParams, EmailData from bugbot.components import ComponentName, fetch_component_teams -# Shared with the rest of bugbot rather than redeclared as ("S1", "S2") the way the -# REO queries have it. It also carries the pre-S1 names — critical maps to S1, major -# and blocker to S2 (see `constants.OLD_SEVERITY_MAP`) — so an old bug that never had -# its severity restated still lands in the S2+ counts instead of quietly missing from -# them. That is a wider net than bugdash casts, so these numbers can run slightly -# ahead of the REO tab's. +# Also matches the pre-S1 severity names, so these counts can run slightly ahead +# of the REO tab's. from bugbot.constants import HIGH_SEVERITY -# The channel this rule posts to. Here rather than in `configs/rules.json` -# because it is not a secret, and because changing where an unattended recurring -# message lands should take a code review -- the same reasoning `frontend_triage` -# gives for keeping its component list in code. The bot token is the part that is -# a secret, and that stays in `configs/config.json`. -# -# TEMPORARY: this is currently #tmp-dm-test, a scratch channel for shaking the -# port out. It has to be pointed at the real REO channel before this message is -# meant for anyone to read. +# TEMPORARY: #tmp-dm-test, a scratch channel for shaking the port out. It has to +# be pointed at the real REO channel before this message is meant to be read. CHANNEL = "C0BLP0WUBED" BZ_BUGLIST_URL = "https://bugzilla.mozilla.org/buglist.cgi" @@ -67,17 +43,9 @@ WELLNESS_API_URL = "https://whattrainisitnow.com/api/wellness/days/" -# The wellness endpoint answers quickly; the Bugzilla searches get bugbot's own -# `bz_query_timeout`, which is far longer. HTTP_TIMEOUT_SECONDS = 15 -# Every Bugzilla classification except Graveyard, which holds the ~100 retired -# products. Same list bugdash's REO queries use. -# -# "Developer Infrastructure" appearing here and in EXCLUDED_PRODUCTS is not a -# contradiction: the classification also holds Firefox Build System, Conduit and -# Tree Management, which stay in scope, and only the product of the same name is -# dropped. +# Every classification except Graveyard, as bugdash's REO queries have it. CLASSIFICATIONS = [ "Client Software", "Components", @@ -86,69 +54,45 @@ "Server Software", ] -# The severity that means no triage decision has been made yet. Bugs are filtered -# on this locally, so the value has to be exactly what Bugzilla reports in a bug's -# severity field, which is case sensitive and not always what the same value looks -# like in a search: "N/A" comes back from the API where a query matches it as -# "n/a". Only "--" counts as missing here; N/A is a decision, not the absence of -# one. +# Bugzilla reports a bug with no triage decision as "--". N/A is a decision, and +# comes back from the API capitalised even though a query matches it as "n/a". MISSING_SEVERITIES = ("--",) -# Products dropped from every query, so their bugs reach no bucket in this -# message. -# -# Not the `exclude_products` key some rules carry in `configs/rules.json`: that one -# subtracts from `BzCleaner`'s default product list, which these classification -# scoped queries never use, so the name would mean something different here. +# Dropped from every query. The Developer Infrastructure classification stays in +# scope; only the product of the same name goes. EXCLUDED_PRODUCTS = ("Testing", "Developer Infrastructure") -# Where the product exclusions are numbered from in a boolean chart. Above every -# slot `regressions_query` uses, including the 11 `with_severities` takes. +# Above every slot the query uses, including the 11 `with_severities` takes. EXCLUDED_PRODUCTS_SLOT = 12 -# For a component with no team_name, or one missing from the mapping entirely. -# Every component had a team when this was written, so this is only a guard -# against silently dropping bugs out of the per-team line. UNKNOWN_TEAM = "Unknown team" # A Slack section block holds at most 3000 characters. SECTION_LIMIT = 3000 -# Above this length a snapshot URL is shortened, and failing that swapped for the -# query URL or dropped entirely -- see `bug_link`. Keeps one very long bug list -# from pushing a section over SECTION_LIMIT. +# Above this length a snapshot URL is shortened; see `bug_link`. MAX_SNAPSHOT_URL = 2000 # Slack has no nested lists in message text, so indent sub-bullets by hand. -# Four plain spaces; if Slack ever collapses them, non-breaking spaces (U+00A0) -# are the fix. SUB_BULLET = " ◦ " -# What a bug search has to come back with for this message. -# -# `groups` is how a bug is known to be restricted, and the message counts those. -# See `restricted_note`. -# -# No `summary` field. That is the line this message does not cross, and the same -# one `BzCleaner.get_summary` draws. +# `groups` is how a bug is known to be restricted. No `summary`: the message +# names no bug, so restricted ones are counted and linked but never named. BUG_FIELDS = "id,severity,product,component,groups" # Stands in for a milestone key, as the last beta is numbered differently from # one version to the next (beta_10 for 154, beta_5 under the 2 week cadence). LAST_BETA = "last_beta" -# The milestone that ends each channel's cycle, and the cycle's name. Both the -# countdown ("End of Beta ...") and the finished line ("Beta cycle finished") are -# built from that one name, so they can't drift apart. Release has no equivalent -# deadline, so it gets no countdown. +# The milestone ending each channel's cycle, and the cycle's name. Release has no +# equivalent deadline, so it gets no countdown. CYCLE_ENDS = { "beta": ("Beta", LAST_BETA), "nightly": ("Nightly", "merge_day"), } -# Custom emoji in the Mozilla workspace, one per channel. A name that doesn't exist -# there renders as the literal :name: rather than failing, so these have to stay -# in step with the workspace. +# Custom emoji in the Mozilla workspace. A name that doesn't exist there renders +# as the literal :name: rather than failing. CHANNEL_EMOJI = { "release": ":firefox-browser:", "beta": ":beta-browser:", @@ -157,8 +101,7 @@ HEADING = "REO release regression status:" -# Shown instead of dropping a channel entirely, so a silent channel reads as -# good news rather than as the script having failed. +# So a silent channel reads as good news rather than as the script having failed. NOTHING_TO_REPORT = "• No open release regressions" @@ -170,9 +113,8 @@ def utc_today() -> datetime.date: def without_excluded_products(slot: int = EXCLUDED_PRODUCTS_SLOT) -> dict: """Chart conditions dropping EXCLUDED_PRODUCTS, a numbered slot per product. - One ANDed notequals per product rather than a single nowords: Bugzilla splits - a nowords value on whitespace, so "Developer Infrastructure" would be matched - as the two words separately and drop products nobody asked to exclude. + One notequals per product rather than a nowords, which Bugzilla would split on + whitespace and match "Developer Infrastructure" as two words. """ conditions: dict = {} for offset, product in enumerate(EXCLUDED_PRODUCTS): @@ -198,21 +140,12 @@ def regressions_query(version: int, carry_over: bool | None = None) -> dict: - stalled or intermittent-failure keywords - within one of EXCLUDED_PRODUCTS - carry_over adds a condition on the previous version, splitting that set in - two. False keeps the bugs where status-firefox{version - 1} is one of - unaffected, ? or ---, so they regressed during this cycle; True negates it, - leaving the ones that were already there. The two therefore partition every - open regression affecting the version, and the default of None asks for that - whole set instead of one side of it. - - Nothing here filters on `bug_group`: an authenticated search returns every bug - the key can see, so restricted regressions arrive on their own. - - Field numbering is Bugzilla's boolean charts: f/o/v are the field, operator - and value for a numbered condition, OP and CP open and close a group, j sets - how a group joins (OR here, AND otherwise) and n negates. The gaps at f7 and - f9 are harmless, as Bugzilla ignores unused numbers: f7 comes from bugdash, - f9 from the product exclusions moving to EXCLUDED_PRODUCTS_SLOT. + carry_over splits that set in two: False keeps the bugs where + status-firefox{version - 1} is unaffected, ? or ---, so they regressed during + this cycle, and True negates it. None asks for the whole set. + + The gaps in the chart numbering come from bugdash and are harmless, as + Bugzilla ignores unused numbers. """ query = { "classification": CLASSIFICATIONS, @@ -234,8 +167,6 @@ def regressions_query(version: int, carry_over: bool | None = None) -> dict: if carry_over is None: return query - # Conditions are matched up by their number, so leaving these out above and - # adding them here changes nothing but the order they appear in the URL. previous = utils.get_flag(version - 1, "status", "release") query |= { "f2": "OP", @@ -253,8 +184,7 @@ def regressions_query(version: int, carry_over: bool | None = None) -> dict: } if carry_over: - # n2 attaches to the OP at f2, so it negates the whole f3-f5 group rather - # than just the first condition in it. + # n2 attaches to the OP at f2, so it negates the whole f3-f5 group. query["n2"] = "1" return query @@ -263,12 +193,7 @@ def regressions_query(version: int, carry_over: bool | None = None) -> dict: def with_severities(query: dict, severities: Collection[str]) -> dict: """Narrow a query to some severities, for a link that stays live. - The counts themselves are filtered locally, so this is only needed to build a - URL when a bug list is too long to link by id. Slot 11 is free: the regressions - query leaves it unused, and EXCLUDED_PRODUCTS_SLOT starts above it. - - Sorted so the same set of severities always produces the same URL: the order a - set iterates in is not stable from one process to the next, and `HIGH_SEVERITY` + Sorted so the same severities always produce the same URL, as `HIGH_SEVERITY` is a set. """ return { @@ -280,19 +205,11 @@ def with_severities(query: dict, severities: Collection[str]) -> dict: def snapshot_url(bugs: list[dict]) -> str: - """A Bugzilla URL listing exactly these bugs, as bugdash's bug lists do. - - Linking the bug ids rather than the query means the list still matches the - count in the message when it is read days later. order=bug_list keeps - Bugzilla showing them in the order given rather than re-sorting. + """A Bugzilla URL listing exactly these bugs, so it still matches the count. Built by hand rather than through `utils.get_bz_search_url` so the separators - stay as commas: percent-encoded they would triple in length, and the length is - what `MAX_SNAPSHOT_URL` is measuring. - - Restricted bugs are in here with everything else. A reader without access gets - a shorter list than the count that linked them here, which is what the - "(n restricted)" note on the bullet is for. + stay as commas: percent-encoded they would triple the length + `MAX_SNAPSHOT_URL` measures. """ ids = ",".join(str(bug["id"]) for bug in bugs) @@ -302,14 +219,9 @@ def snapshot_url(bugs: list[dict]) -> str: def shortened_url(url: str) -> str | None: """A short Bugzilla URL for a long one, or None if it couldn't be shortened. - `utils.shorten_long_bz_url` answers a shortener error by returning the URL - split across several lines (bugbot#1402). Harmless in an email, useless in a - Slack link, which would end at the first newline — so a multi-line answer is - treated as a failure here rather than posted. - - Any other failure is swallowed for the same reason: the count is the message - and the link is a convenience, so a shortener that is down should cost the link - and nothing more. + `utils.shorten_long_bz_url` answers an error with the URL split across lines + (bugbot#1402), which a Slack link would end at the first newline, so that + counts as a failure too. """ try: short = utils.shorten_long_bz_url(url) @@ -328,16 +240,10 @@ def bug_link( ) -> str: """Format a non-empty bug list as a Slack link labelled with its count. - label_template is formatted with the count, e.g. "{} New Regressions". - - A snapshot URL that comes out too long is shortened, which keeps the link - pointing at exactly the bugs counted. Failing that it falls back to - fallback_query, which is a live query and so can drift from the count beside - it, and failing that the count is left unlinked. Team lines pass no fallback, - as reproducing a team as a query means listing all its components. - - Callers are expected to skip empty lists: an empty bug_id would link to a - broken list, and a count of zero is left out of the message anyway. + An over-long snapshot URL is shortened, then falls back to fallback_query, + which is live and so can drift from the count, then to no link at all. Team + lines pass no fallback, as reproducing a team as a query means listing all its + components. """ label = label_template.format(len(bugs)) snapshot = snapshot_url(bugs) @@ -357,12 +263,7 @@ def bug_link( @functools.cache def component_teams() -> dict[ComponentName, str]: - """Map every (product, component) to the team that owns it. - - team_name is a Bugzilla field on components, the same one bugdash's Teams - filter uses. One request covers every product, around 120KB for 2000-odd - components, which is why it's cached for the life of the run. - """ + """Map every (product, component) to the team that owns it, in one request.""" return fetch_component_teams() @@ -372,11 +273,7 @@ def team_of(bug: dict) -> str: def team_breakdown(bugs: list[dict]) -> str: - """Count the bugs owned by each team, busiest team first. - - Every team is listed rather than just the top few, so that the line works - as a nudge to each team that owns something. - """ + """Count the bugs owned by each team, busiest team first.""" by_team: dict[str, list[dict]] = {} for bug in bugs: by_team.setdefault(team_of(bug), []).append(bug) @@ -389,15 +286,8 @@ def team_breakdown(bugs: list[dict]) -> str: def restricted_note(bugs: list[dict]) -> str: """Say how many of a bug list are restricted, or nothing when none are. - A bug is restricted when it is in any group at all, not only a security one: - the note exists to explain why the linked list looks shorter than the count to - a reader without access, and that gap opens for an employee-confidential or - partner group just as it does for `core-security`. - - Deliberately plain text rather than part of the link label, so the blue runs as - far as the thing being counted and no further, and deliberately only used on the - top-level bullets: repeated on every severity and team sub-bullet it would say - little and crowd out the counts that are the point of those lines. + Any group counts, not only a security one: this explains why the linked list + looks shorter than the count to a reader without access. """ count = sum(1 for bug in bugs if bug.get("groups")) if not count: @@ -409,15 +299,9 @@ def restricted_note(bugs: list[dict]) -> str: def to_blocks(sections: list[str]) -> list[dict]: """Wrap the sections of a message as Block Kit sections. - Slack silently splits a message whose text runs past about 4000 characters - into several messages, which is what happened when every count linked to a - full query URL. Snapshot URLs brought the total well under that, but each - section block gets its own 3000 character allowance, so keeping the sections - means a busier cycle can't start splitting the message again. - - A section that does overflow raises rather than posting something malformed. - The team breakdown is the part that could get there, at roughly 90 characters - per team; capping or splitting it is the fix if that ever fires. + Slack silently splits a message past about 4000 characters, where each section + block gets its own allowance. An overflowing section raises rather than + posting something malformed. """ for section in sections: if len(section) > SECTION_LIMIT: @@ -433,12 +317,7 @@ def to_blocks(sections: list[str]) -> list[dict]: def block_text(block: dict) -> str: - """The text of any block, for printing a message instead of posting it. - - Section and header blocks keep their text in one place and context blocks in - a list of elements, so a dry run has to handle both rather than assume the - shape of the blocks it was handed. - """ + """The text of any block, for printing a message instead of posting it.""" if "elements" in block: return " ".join(element["text"] for element in block["elements"]) @@ -446,12 +325,10 @@ def block_text(block: dict) -> str: def versions_to_report() -> dict[str, int]: - """The current version of each channel, with what was read written to the log. + """The current version of each channel, logging what was read. - From the trains API rather than through `BzCleaner.init_versions`: - `utils.get_checked_versions` returns nothing on merge day, and - `has_enough_data` would then skip the run on exactly the day this message - has its own wording for. + Not through `BzCleaner.init_versions`: `utils.get_checked_versions` returns + nothing on merge day, which is a day this message has wording for. """ versions = utils.get_versions_from_trains() logger.info( @@ -468,9 +345,7 @@ def versions_to_report() -> dict[str, int]: def wellness_days() -> frozenset[datetime.date]: """Fetch the days off that don't count as working days. - libmozdata's `FirefoxTrains` covers the schedule and owners endpoints but not - this one, so it is fetched directly. Moving it there is the tidier home if a - second caller ever turns up. + libmozdata's `FirefoxTrains` doesn't cover this endpoint. """ response = requests.get( WELLNESS_API_URL, @@ -485,9 +360,8 @@ def wellness_days() -> frozenset[datetime.date]: def work_days_until(end: datetime.date) -> int: """Count working days between today and end, end excluded. - Mirrors ReleaseInsights\\Duration::workDays() so this agrees with the - countdowns on the release pages: weekends, wellness days and the current - day are all left out. + Mirrors ReleaseInsights\\Duration::workDays(), so this agrees with the + countdowns on the release pages. """ today = utc_today() days = (end - today).days @@ -511,9 +385,7 @@ def release_schedule(version: int) -> dict: def milestone_date(schedule: dict, milestone: str) -> datetime.date: """The date of a milestone, resolving LAST_BETA to the highest numbered beta. - The number of betas differs per version, so the last one has to be found - rather than named. Sorting on the number matters: as strings, beta_9 would - come after beta_10. + Sorting on the number matters: as strings, beta_9 would come after beta_10. """ if milestone == LAST_BETA: betas = [key for key in schedule if re.fullmatch(r"beta_\d+", key)] @@ -525,12 +397,8 @@ def milestone_date(schedule: dict, milestone: str) -> datetime.date: def cycle_countdown(version: int, channel: str) -> str: """A countdown to the end of this version's time on the channel. - Beta ends with the last beta build; Nightly ends on merge day, when the - version moves to Beta. Release has no such deadline. - - The version numbers roll over on merge day, so the day of and the days after - that deadline each only show up briefly, but they read badly as a countdown - ("in 0 working days") and so get their own wording. + The deadline and the days after it read badly as a countdown ("in 0 working + days"), so they get their own wording. """ if channel not in CYCLE_ENDS: return "" @@ -554,36 +422,28 @@ def cycle_countdown(version: int, channel: str) -> str: class ReoRegressionSlack(BzCleaner): - """Post the state of this cycle's open release regressions to Slack. + """A `BzCleaner` that reports to Slack instead of by email. - A `BzCleaner` that reports to Slack instead of by email: the searches, the - arguments, the `must_run` gate and the error handling are all the - framework's, and `get_email_data` posts the message and returns nothing to - mail. The days it runs on are `must_run` in `configs/rules.json`. + The days it runs on are `must_run` in `configs/rules.json`. """ - # Where the message goes. A `--channel` run overrides it, so this is the - # channel the cron posts to; see `parse_custom_arguments`. + # Overridden by a `--channel` run, so this is the channel the cron posts to. channel = CHANNEL def description(self) -> str: return "REO release regression cycle summary posted to Slack" def all_include_fields(self) -> bool: - # The fields a search asks for are `BUG_FIELDS` and nothing else. - # `BzCleaner` would otherwise add `summary` to every query, which is the - # one field this message does not print -- see `restricted_note`. + # `BzCleaner` would otherwise add `summary` to every query. return True def has_default_products(self) -> bool: - # The query is scoped by classification, as bugdash's REO queries are; - # the default product list would report a different bug set. + # Scoped by classification instead, as bugdash's REO queries are. return False def filter_no_nag_keyword(self) -> bool: - # This message counts bugs rather than nagging about them, and a - # [no-nag] bug is still one the cycle is carrying. Dropping those would - # put the counts out of step with the REO tab. + # A [no-nag] bug is still one the cycle is carrying, and dropping those + # would put the counts out of step with the REO tab. return False def add_custom_arguments(self, parser: argparse.ArgumentParser) -> None: @@ -605,29 +465,16 @@ def get_bz_params(self, date: str) -> BzParams: return self.params def bughandler(self, bug: Bug, data: dict[str, Any]) -> None: - """Keep every field of the bug, keyed by its id. - - `BzCleaner`'s own handler reduces a bug to the columns of an email - table, its summary included. This message reports counts and - breakdowns, so it needs the fields it asked for and none of the rest. - """ + """Keep every field, where `BzCleaner` would keep the email columns.""" data[str(bug["id"])] = bug def fetch_bugs(self, query: dict, fields: str = BUG_FIELDS) -> list[dict]: """Run one of this rule's queries through `BzCleaner`'s search path. - Several queries per run -- two per channel -- each one set here and read - back by `get_bz_params`, the way `warn_regressed_by` steps through its - two. Going through `get_bugs` is what attaches bugbot's API key, which - is the whole reason this sees restricted bugs, along with the query - timeout and the paging. libmozdata pages a search itself -- counting - first, then walking the results in chunks -- but only for a query - carrying none of count_only, limit, order or offset, so no query here - may add one. - - Fetching the bugs rather than asking for count_only is what lets the - severity and team breakdowns be derived from one request, and lets each - count link to the exact bugs behind it. + Two queries per channel, each set here and read back by `get_bz_params`, + the way `warn_regressed_by` steps through its two. libmozdata only pages a + query carrying none of count_only, limit, order or offset, so no query + here may add one. """ self.params = {**query, "include_fields": fields} @@ -638,13 +485,9 @@ def regression_group( ) -> str: """Build the bullet and severity sub-bullets for one bug list. - The list is fetched once and split by severity and team here, rather than - asking Bugzilla for each subset, so the sub-bullets are guaranteed to be - part of the count above them. - - Bug lists that are empty are left out entirely rather than reported as a - zero, so a quiet channel is short instead of a wall of "0". Returns an - empty string when there are no bugs at all. + Split locally rather than queried per subset, so the sub-bullets are + guaranteed to be part of the count above them. Empty lists are left out of + the message entirely. """ query = regressions_query(version, carry_over) bugs = self.fetch_bugs(query) @@ -674,33 +517,20 @@ def regression_group( return "\n".join(lines) def post_message(self, blocks: list[dict]) -> None: - """Post the message to Slack, or print it when the run isn't for real. - - A dry run prints what it would have posted, so `--production` means here - what it means for every other rule. `test_mode` is honoured alongside it - for the reason `triage_owner_rotations` honours it: a test run must reach - nobody. - """ + """Post the message to Slack, or print it on a dry or test run.""" if self.dryrun or self.test_mode: print("DRY RUN: message not posted.\n") for block in blocks: print(block_text(block)) return - # HEADING is the message's notification fallback text, which is what a - # client that cannot render blocks shows instead of them. + # HEADING is the notification fallback text, which is what a client that + # cannot render blocks shows instead of them. slack.post_to_slack(self.channel, HEADING, blocks=blocks) logger.info("Rule %s posted to %s", self.name(), self.channel) def get_email_data(self, date: str) -> EmailData: - """Post the message, and give `send_email` nothing to send. - - The report is the Slack message rather than an email, and an empty list - is what stops one being sent -- the same way `security_affected_versions` - runs the pipeline for the needinfos it posts and mails no summary. The - "No data" line `send_email` then logs is about that email, not about the - message, which has been posted by the time it is written. - """ + """Post the message, and return no data so `send_email` sends nothing.""" self.post_message(self.blocks(versions_to_report())) return [] diff --git a/bugbot/rules/reo_regression_slack_daily.py b/bugbot/rules/reo_regression_slack_daily.py index 678b680a9..fd1e8465f 100644 --- a/bugbot/rules/reo_regression_slack_daily.py +++ b/bugbot/rules/reo_regression_slack_daily.py @@ -4,23 +4,15 @@ """Post the REO release regressions that need action to Slack. -The same bug set as the cycle summary, `reo_regression_slack`, with the new and -carry over split dropped and the three channels merged into one deduplicated -list, reporting only the bugs stuck long enough to need a nudge: high severity -with nobody on them (UNASSIGNED_EXEMPT_* exempt), no severity decision, or an +The same bug set as the cycle summary, `reo_regression_slack`, with the three +channels merged into one deduplicated list and only the bugs stuck long enough +to need a nudge: high severity with nobody on them, no severity decision, or an unanswered needinfo. It ends with bugdash's Burndown list per version, Beta and Release only, cut down to the fixes nobody has asked to uplift. Each line is broken down by owning team. -Posted every weekday the cron script invokes it, unlike the summary: these are -things somebody has to do, so a day skipped is a day nobody was asked. - -Every count links to a Bugzilla list of exactly the bugs counted. Restricted -bugs are counted in the totals and included in the links like any other, but -never named: no message prints a bug summary, which is the same line -`BzCleaner.get_summary` draws. The top-level bullet says how many of its count -are restricted, because a reader without access opens the link and finds a -shorter list than the number they clicked on. See `restricted_note`. +Posted every weekday, unlike the summary: these are things somebody has to do, +so a day skipped is a day nobody was asked. """ import argparse @@ -34,34 +26,17 @@ from bugbot.bzcleaner import Bug, BzCleaner, BzParams, EmailData from bugbot.components import ComponentName, fetch_component_teams -# Shared with the rest of bugbot rather than redeclared as ("S1", "S2") the way the -# REO queries have it. It also carries the pre-S1 names — critical maps to S1, major -# and blocker to S2 (see `constants.OLD_SEVERITY_MAP`) — so an old bug that never had -# its severity restated still lands in the S2+ counts instead of quietly missing from -# them. That is a wider net than bugdash casts, so these numbers can run slightly -# ahead of the REO tab's. +# Also matches the pre-S1 severity names, so these counts can run slightly ahead +# of the REO tab's. from bugbot.constants import HIGH_SEVERITY -# The channel this rule posts to. Here rather than in `configs/rules.json` -# because it is not a secret, and because changing where an unattended recurring -# message lands should take a code review -- the same reasoning `frontend_triage` -# gives for keeping its component list in code. The bot token is the part that is -# a secret, and that stays in `configs/config.json`. -# -# TEMPORARY: this is currently #tmp-dm-test, a scratch channel for shaking the -# port out. It has to be pointed at the real REO channel before this message is -# meant for anyone to read. +# TEMPORARY: #tmp-dm-test, a scratch channel for shaking the port out. It has to +# be pointed at the real REO channel before this message is meant to be read. CHANNEL = "C0BLP0WUBED" BZ_BUGLIST_URL = "https://bugzilla.mozilla.org/buglist.cgi" -# Every Bugzilla classification except Graveyard, which holds the ~100 retired -# products. Same list bugdash's REO queries use. -# -# "Developer Infrastructure" appearing here and in EXCLUDED_PRODUCTS is not a -# contradiction: the classification also holds Firefox Build System, Conduit and -# Tree Management, which stay in scope, and only the product of the same name is -# dropped. +# Every classification except Graveyard, as bugdash's REO queries have it. CLASSIFICATIONS = [ "Client Software", "Components", @@ -70,121 +45,62 @@ "Server Software", ] -# The severity that means no triage decision has been made yet. Bugs are filtered -# on this locally, so the value has to be exactly what Bugzilla reports in a bug's -# severity field, which is case sensitive and not always what the same value looks -# like in a search: "N/A" comes back from the API where a query matches it as -# "n/a". Only "--" counts as missing here; N/A is a decision, not the absence of -# one. +# Bugzilla reports a bug with no triage decision as "--". N/A is a decision, and +# comes back from the API capitalised even though a query matches it as "n/a". MISSING_SEVERITIES = ("--",) -# Products dropped from every query, so their bugs reach no bucket in this -# message and none of the burndown lines. -# -# Not the `exclude_products` key some rules carry in `configs/rules.json`: that one -# subtracts from `BzCleaner`'s default product list, which these classification -# scoped queries never use, so the name would mean something different here. +# Dropped from every query. The Developer Infrastructure classification stays in +# scope; only the product of the same name goes. EXCLUDED_PRODUCTS = ("Testing", "Developer Infrastructure") -# Where the product exclusions are numbered from in a boolean chart. Above every -# slot either query uses -- `regressions_query` and `burndown_query`. +# Above every slot either query uses. EXCLUDED_PRODUCTS_SLOT = 12 -# For a component with no team_name, or one missing from the mapping entirely. -# Every component had a team when this was written, so this is only a guard -# against silently dropping bugs out of the per-team line. UNKNOWN_TEAM = "Unknown team" # A Slack section block holds at most 3000 characters. SECTION_LIMIT = 3000 -# Above this length a snapshot URL is shortened, and failing that dropped -# entirely -- see `bug_link`. Keeps one very long bug list from pushing a section -# over SECTION_LIMIT. +# Above this length a snapshot URL is shortened; see `bug_link`. MAX_SNAPSHOT_URL = 2000 -# Slack renders this back as >. Sending the character itself would work where it -# is used now, but it ends a link's label at the first > and opens a blockquote at -# the start of a line, so a label or bullet reworded around it would break in ways -# that are easy to miss. The entity is never wrong. +# Slack renders this back as >. The bare character ends a link's label and opens +# a blockquote at the start of a line. GREATER_THAN = ">" # Slack has no nested lists in message text, so indent sub-bullets by hand. -# Four plain spaces; if Slack ever collapses them, non-breaking spaces (U+00A0) -# are the fix. SUB_BULLET = " ◦ " -# Products where an unassigned high severity bug is not something to nag about, -# so they are left out of the "S2+ unassigned" bucket alone. Empty today, as the -# one exemption we have belongs to a component rather than to a whole product; -# kept so exempting a product later is a one line change. +# `groups` is how a bug is known to be restricted. No `summary`: the message +# names no bug, so restricted ones are counted and linked but never named. +BUG_FIELDS = "id,severity,product,component,groups" + +# What each query adds on top: whatever its buckets age a bug from. +FIELDS = f"{BUG_FIELDS},assigned_to,creation_time,last_change_time,flags" +BURNDOWN_FIELDS = f"{BUG_FIELDS},cf_last_resolved" + +# Left out of the "S2+ unassigned" bucket alone. Empty today, as the one +# exemption we have belongs to a component rather than to a whole product. UNASSIGNED_EXEMPT_PRODUCTS: tuple[str, ...] = () -# The same, per component: Web Compatibility::Site Reports bugs S2 definition -# does not follow the regression severity definition. The exemption is the -# component's, not the product's — Site Reports only happens to sit under Web -# Compatibility, and the rest of that product still follows the definition. -# -# Matched on the component name alone, exactly as Bugzilla reports it, and only -# against this one bucket: an exempt bug with no severity or an unanswered -# needinfo is still stuck in the way those buckets mean. +# Web Compatibility::Site Reports bugs S2 definition does not follow the +# regression severity definition. The exemption is the component's, not the +# product's, and applies to this one bucket only. UNASSIGNED_EXEMPT_COMPONENTS = ("Site Reports",) -# How long a bug has to have been stuck before this message nags about it. -# Long enough that a bug filed or touched during yesterday's working day is left -# alone, short enough that nothing sits unnoticed for a second day. -# -# It ages bugs from a fixed point in the past rather than over a window, so a -# quiet weekend doesn't hide anything: a bug that went stale on Friday is still -# in Monday's message, and stays there until someone acts on it. +# Bugs are aged from a fixed point in the past rather than over a window, so one +# that went stale on Friday is still in Monday's message. STUCK_HOURS = 24 -# The channels a fix has to be uplifted to reach. A fix only reaches Beta or -# Release by being uplifted, so a burndown bug with no uplift request against the -# channel is a fix that will not ship in the version it is marked as affecting. -# -# The flag is matched by name alone, so any state of it counts as asked: pending -# (?), granted (+) and denied (-) alike. Matching only a pending request would -# put a bug back on the list the moment its uplift was approved, since the flag -# stops being pending then and the fix has yet to land, and would keep a denied -# one on the list for good. -# -# Nightly is where fixes land, so it needs no uplift and gets no burndown line. -# The order here is the order the lines appear in. -# -# A channel added here also needs a version from `utils.get_versions_from_trains()`. -# One without a version is skipped with a note in the log rather than reported. -# -# The flag name itself is built by `utils.get_flag`, so this is only the list of -# channels. That is also what makes ESR addable: its flag embeds the version -# number, which `get_flag` knows how to format and a constant here could not. +# The channels a fix only reaches by being uplifted, in the order the burndown +# lines appear. Nightly is where fixes land, so it needs no line. UPLIFT_CHANNELS = ("beta", "release") -# The title leads with what makes this message different from the twice weekly -# status summary, rather than trailing it. Slack cuts a long title off in -# notification previews and the eye reads from the left, so a title starting -# "REO release regression" like the other one would be indistinguishable at a -# glance. It is also the fallback text of the message, which is what those -# previews show. -# -# Slack allows 150 characters in a header block, which this is nowhere near. +# Also the message's fallback text, which is what notification previews show. HEADING = "Action needed: REO release regressions" -# Sits under the heading in a context block: small, grey, and read as a label on -# the message rather than as part of it. Says the message is a recurring one, so -# a reader who has not seen it before knows it is not an incident. CADENCE = "Daily update" -# Follows the heading. Says the one thing every line below has in common, so the -# bullets don't each have to explain themselves, and points each team at the -# sub-bullets, which is where the message asks anything of anyone. -# -# What the buckets share is that none of them is waiting on the work: each is -# waiting on an action, which is what makes the message worth sending daily and -# what separates it from the twice weekly summary of how the cycle is going. -# -# The age is given here as a round number and again on each bullet, where it is -# also said what the age is counted from, as that differs per bucket. INTRO = ( "These release regressions are waiting on activity and fall into the urgent " "category. " @@ -192,30 +108,15 @@ "Please take a look where one of your teams is listed." ) -# Shown on a day where every bucket came out empty, so a quiet day reads as good -# news rather than as the script having failed. +# So a quiet day reads as good news rather than as the script having failed. NOTHING_STUCK = "• Nothing needs attention" -# What a bug search has to come back with for this message: it ages every bug, -# and the three timestamps it can age one from all live on the bug itself, so -# asking for them keeps it to the same one request per version. -# -# `groups` is how a bug is known to be restricted, and the message counts those. -# See `restricted_note`. -# -# No `summary` field, in either of these. That is the line this message does not -# cross, and the same one `BzCleaner.get_summary` draws. -BUG_FIELDS = "id,severity,product,component,groups" -FIELDS = f"{BUG_FIELDS},assigned_to,creation_time,last_change_time,flags" -BURNDOWN_FIELDS = f"{BUG_FIELDS},cf_last_resolved" - def without_excluded_products(slot: int = EXCLUDED_PRODUCTS_SLOT) -> dict: """Chart conditions dropping EXCLUDED_PRODUCTS, a numbered slot per product. - One ANDed notequals per product rather than a single nowords: Bugzilla splits - a nowords value on whitespace, so "Developer Infrastructure" would be matched - as the two words separately and drop products nobody asked to exclude. + One notequals per product rather than a nowords, which Bugzilla would split on + whitespace and match "Developer Infrastructure" as two words. """ conditions: dict = {} for offset, product in enumerate(EXCLUDED_PRODUCTS): @@ -242,18 +143,10 @@ def regressions_query(version: int) -> dict: - within one of EXCLUDED_PRODUCTS The cycle summary splits this set into the bugs that regressed during the - cycle and the ones that were already there; this message wants the whole set, - so it has no equivalent of that condition. - - Nothing here filters on `bug_group`: an authenticated search returns every bug - the key can see, so restricted regressions arrive on their own. - - Field numbering is Bugzilla's boolean charts: f/o/v are the field, operator - and value for a numbered condition, OP and CP open and close a group, j sets - how a group joins (OR here, AND otherwise) and n negates. The gaps at f2-f7 - and f9 are harmless, as Bugzilla ignores unused numbers: f7 comes from - bugdash, f2-f6 are where the cycle summary puts its split, and f9 is free now - the product exclusions live at EXCLUDED_PRODUCTS_SLOT. + cycle and the ones that were already there; this message wants the whole set. + + The gaps in the chart numbering come from bugdash and from that split, and + are harmless: Bugzilla ignores unused numbers. """ return { "classification": CLASSIFICATIONS, @@ -288,25 +181,11 @@ def burndown_query(version: int, uplift_flag: str) -> dict: - an uplift request against the channel, in any state All but the last of those is bugdash's Burndown list, kept in step with - app/buglists/burndown.mjs there. Its numbering gaps at f5, f8 and f10 are - copied along with the rest, as Bugzilla ignores unused numbers, and f9 is - free now the product exclusions live at EXCLUDED_PRODUCTS_SLOT. - - The f3-f7 group is what narrows "every fix still marked affected" down to the - fixes worth chasing an uplift for, and being a security bug is one of the three - ways in. It is a test of whether a bug qualifies, not of whether we can see it: - under anonymous queries that branch matched nothing, because a search never - returns a bug the requester cannot read, so it only starts contributing here. - What it adds is the fixed security bugs carrying none of those keywords and no - tracking flag — a population that would otherwise fall off the burndown despite - being perfectly visible. - - The uplift request is a flag on an attachment, and the only way a bug search - will report those is to send back every attachment with it, so it is left to - Bugzilla rather than filtered here. flagtypes.name matches the flags on a - bug's attachments as well as those on the bug itself, on name and state - together, so matching the bare name catches the request whatever became of - it. n11 negates that, leaving the fixes nobody has asked to uplift. + app/buglists/burndown.mjs there, its gaps in the chart numbering included. + + The uplift request is a flag on an attachment, so it is left to Bugzilla + rather than filtered here: flagtypes.name matches attachment flags too, and + matching the bare name catches the request in any state. n11 negates that. """ return { "classification": CLASSIFICATIONS, @@ -335,19 +214,11 @@ def burndown_query(version: int, uplift_flag: str) -> dict: def snapshot_url(bugs: list[dict]) -> str: - """A Bugzilla URL listing exactly these bugs, as bugdash's bug lists do. - - Linking the bug ids rather than the query means the list still matches the - count in the message when it is read days later. order=bug_list keeps - Bugzilla showing them in the order given rather than re-sorting. + """A Bugzilla URL listing exactly these bugs, so it still matches the count. Built by hand rather than through `utils.get_bz_search_url` so the separators - stay as commas: percent-encoded they would triple in length, and the length is - what `MAX_SNAPSHOT_URL` is measuring. - - Restricted bugs are in here with everything else. A reader without access gets - a shorter list than the count that linked them here, which is what the - "(n restricted)" note on the bullet is for. + stay as commas: percent-encoded they would triple the length + `MAX_SNAPSHOT_URL` measures. """ ids = ",".join(str(bug["id"]) for bug in bugs) @@ -357,14 +228,9 @@ def snapshot_url(bugs: list[dict]) -> str: def shortened_url(url: str) -> str | None: """A short Bugzilla URL for a long one, or None if it couldn't be shortened. - `utils.shorten_long_bz_url` answers a shortener error by returning the URL - split across several lines (bugbot#1402). Harmless in an email, useless in a - Slack link, which would end at the first newline — so a multi-line answer is - treated as a failure here rather than posted. - - Any other failure is swallowed for the same reason: the count is the message - and the link is a convenience, so a shortener that is down should cost the link - and nothing more. + `utils.shorten_long_bz_url` answers an error with the URL split across lines + (bugbot#1402), which a Slack link would end at the first newline, so that + counts as a failure too. """ try: short = utils.shorten_long_bz_url(url) @@ -381,16 +247,9 @@ def shortened_url(url: str) -> str | None: def bug_link(bugs: list[dict], label_template: str) -> str: """Format a non-empty bug list as a Slack link labelled with its count. - label_template is formatted with the count, e.g. "{} S2+ unassigned". - - A snapshot URL that comes out too long is shortened, which keeps the link - pointing at exactly the bugs counted, and failing that the count is left - unlinked. No line here has a live query to fall back on: the ageing is done - in this rule rather than by Bugzilla, and reproducing a team as a query means - listing all its components. - - Callers are expected to skip empty lists: an empty bug_id would link to a - broken list, and a count of zero is left out of the message anyway. + An over-long snapshot URL is shortened, and failing that the count is left + unlinked. No line here has a live query to fall back on, as the ageing is done + in this rule rather than by Bugzilla. """ label = label_template.format(len(bugs)) snapshot = snapshot_url(bugs) @@ -407,12 +266,7 @@ def bug_link(bugs: list[dict], label_template: str) -> str: @functools.cache def component_teams() -> dict[ComponentName, str]: - """Map every (product, component) to the team that owns it. - - team_name is a Bugzilla field on components, the same one bugdash's Teams - filter uses. One request covers every product, around 120KB for 2000-odd - components, which is why it's cached for the life of the run. - """ + """Map every (product, component) to the team that owns it, in one request.""" return fetch_component_teams() @@ -422,11 +276,7 @@ def team_of(bug: dict) -> str: def team_breakdown(bugs: list[dict]) -> str: - """Count the bugs owned by each team, busiest team first. - - Every team is listed rather than just the top few, so that the line works - as a nudge to each team that owns something. - """ + """Count the bugs owned by each team, busiest team first.""" by_team: dict[str, list[dict]] = {} for bug in bugs: by_team.setdefault(team_of(bug), []).append(bug) @@ -439,18 +289,10 @@ def team_breakdown(bugs: list[dict]) -> str: def restricted_note(bugs: list[dict]) -> str: """Say how many of a bug list are restricted, or nothing when none are. - A bug is restricted when it is in any group at all, not only a security one: - the note exists to explain why the linked list looks shorter than the count to - a reader without access, and that gap opens for an employee-confidential or - partner group just as it does for `core-security`. That is a wider test than - the `bug_group ~ "sec"` branch in `burndown_query`, which is asking a - different question — whether a fix is worth chasing, not whether it is - readable. - - Deliberately plain text rather than part of the link label, so the blue runs as - far as the thing being counted and no further, and deliberately only used on the - top-level bullets: repeated on every team sub-bullet it would say little and - crowd out the counts that are the point of those lines. + Any group counts, not only a security one: this explains why the linked list + looks shorter than the count to a reader without access. That is a wider test + than the `bug_group ~ "sec"` branch in `burndown_query`, which asks whether a + fix is worth chasing rather than whether the bug is readable. """ count = sum(1 for bug in bugs if bug.get("groups")) if not count: @@ -462,15 +304,9 @@ def restricted_note(bugs: list[dict]) -> str: def to_blocks(sections: list[str]) -> list[dict]: """Wrap the sections of a message as Block Kit sections. - Slack silently splits a message whose text runs past about 4000 characters - into several messages, which is what happened when every count linked to a - full query URL. Snapshot URLs brought the total well under that, but each - section block gets its own 3000 character allowance, so keeping the sections - means a busier day can't start splitting the message again. - - A section that does overflow raises rather than posting something malformed. - The team breakdown is the part that could get there, at roughly 90 characters - per team; capping or splitting it is the fix if that ever fires. + Slack silently splits a message past about 4000 characters, where each section + block gets its own allowance. An overflowing section raises rather than + posting something malformed. """ for section in sections: if len(section) > SECTION_LIMIT: @@ -486,12 +322,7 @@ def to_blocks(sections: list[str]) -> list[dict]: def block_text(block: dict) -> str: - """The text of any block, for printing a message instead of posting it. - - Section and header blocks keep their text in one place and context blocks in - a list of elements, so a dry run has to handle both rather than assume the - shape of the blocks it was handed. - """ + """The text of any block, for printing a message instead of posting it.""" if "elements" in block: return " ".join(element["text"] for element in block["elements"]) @@ -499,12 +330,10 @@ def block_text(block: dict) -> str: def versions_to_report() -> dict[str, int]: - """The current version of each channel, with what was read written to the log. + """The current version of each channel, logging what was read. - From the trains API rather than through `BzCleaner.init_versions`: - `utils.get_checked_versions` returns nothing on merge day, and - `has_enough_data` would then skip the run on exactly the day this message - has its own wording for. + Not through `BzCleaner.init_versions`: `utils.get_checked_versions` returns + nothing on merge day, which is a day this message has wording for. """ versions = utils.get_versions_from_trains() logger.info( @@ -525,11 +354,7 @@ def stuck_since() -> datetime.datetime: def unassigned_exempt(bug: dict) -> bool: - """Whether a bug is exempt from the S2+ unassigned bucket. - - Exempt by product or by component, so either can be exempted on its own - without the other having to be named. - """ + """Whether a bug is exempt from the S2+ unassigned bucket.""" return ( bug["product"] in UNASSIGNED_EXEMPT_PRODUCTS or bug["component"] in UNASSIGNED_EXEMPT_COMPONENTS @@ -539,15 +364,9 @@ def unassigned_exempt(bug: dict) -> bool: def needs_assignee(bug: dict, cutoff: datetime.datetime) -> bool: """A high severity bug nobody has taken on, aged from when it was filed. - What counts as unassigned is `utils.is_no_assignee`, shared with the rest of - bugbot: the nobody@ placeholder, a component's `.bugs` default address, or no - assignee at all. That last pair is wider than the REO queries take it — they - count a bug parked on a component default as assigned — so this can flag a bug - bugdash would not. Nothing differed on the day it changed, but that is a fact - about that day's bug set rather than a guarantee. - - Exempt bugs are left out: an unassigned bug there is not a bug that has been - overlooked. See unassigned_exempt(). + `utils.is_no_assignee` is wider than the REO queries take it, which count a + bug parked on a component default address as assigned, so this can flag a bug + bugdash would not. """ return ( bug["severity"] in HIGH_SEVERITY @@ -560,10 +379,8 @@ def needs_assignee(bug: dict, cutoff: datetime.datetime) -> bool: def needs_severity(bug: dict, cutoff: datetime.datetime) -> bool: """A bug still waiting on a severity decision, aged from its last activity. - Any change to the bug counts as activity, not just a triage one, so a bug - with activity is left out until it goes quiet again. There are some limitations - with this approach since the activity may be from someone outside the triage - team asking questions or adjusting metadata. + Any change counts as activity, not just a triage one, so a bug someone is + asking questions on is left out until it goes quiet again. """ return ( bug["severity"] in MISSING_SEVERITIES @@ -572,16 +389,11 @@ def needs_severity(bug: dict, cutoff: datetime.datetime) -> bool: def needs_answer(bug: dict, cutoff: datetime.datetime) -> bool: - """A bug with a needinfo nobody has answered, aged from when it was requested. + """A bug with a needinfo nobody has answered, aged from when it was asked. - What counts as an open request is left to `utils.get_needinfo`, so this agrees - with every other rule that nags about one. The ageing is not: `get_needinfo` - filters on modification_date in whole days, and a flag's creation_date is when - the request now standing was made, so one that was answered and then asked again - is aged from the second ask rather than the first. - - Several open requests on one bug still only count the bug once, and the oldest - of them is what decides. + Several open requests still only count the bug once. Aged from the flag's + creation_date, which is when the request now standing was made, so one asked + again after an answer is aged from the second ask. """ return any( lmdutils.get_date_ymd(flag["creation_date"]) < cutoff @@ -589,15 +401,9 @@ def needs_answer(bug: dict, cutoff: datetime.datetime) -> bool: ) -# The buckets, in the order they appear in the message: what makes a bug belong -# in one, the label its count goes in, and what its age is counted from. A bug -# can be in more than one, as they describe different things left undone rather -# than a state it is in. -# -# Every bucket names its own anchor because each is aged from a different -# timestamp. Left unsaid, the same "> 24 hours" on every bullet reads as one -# shared deadline, when a bug filed weeks ago and one that went quiet yesterday -# are being asked about for different reasons. +# What makes a bug belong in a bucket, the label its count goes in, and what its +# age is counted from. A bug can be in more than one. Each names its own anchor +# because each is aged from a different timestamp. STUCK_BUCKETS = ( (needs_assignee, "{} S2+ unassigned", "filed"), (needs_severity, "{} missing severity", "last change"), @@ -609,12 +415,8 @@ def stuck_group(bugs: list[dict], label: str, anchor: str) -> str: """Build the bullet and team sub-bullet for one bucket. Only the count and what it counts are linked; the restricted note and the age - that follow are left as plain text, so the blue runs as far as the thing being - claimed and no further. Building that tail here is what keeps every bullet the - same shape, the burndown lines included. - - Empty buckets return an empty string and are left out of the message, so it - stays a list of things to do rather than a scoreboard of zeros. + that follow are plain text. Empty buckets return an empty string and are left + out of the message. """ if not bugs: return "" @@ -628,40 +430,29 @@ def stuck_group(bugs: list[dict], label: str, anchor: str) -> str: class ReoRegressionSlackDaily(BzCleaner): - """Post the release regressions that are waiting on somebody to Slack. + """A `BzCleaner` that reports to Slack instead of by email. - A `BzCleaner` that reports to Slack instead of by email: the searches, the - arguments and the error handling are all the framework's, and - `get_email_data` posts the message and returns nothing to mail. - - No `must_run` entry in `configs/rules.json`: this one runs every day the - cron script invokes it, which is every weekday. The twice weekly summary, - `reo_regression_slack`, is the one with a cadence of its own. + No `must_run` in `configs/rules.json`: this runs every day the cron invokes it. """ - # Where the message goes. A `--channel` run overrides it, so this is the - # channel the cron posts to; see `parse_custom_arguments`. + # Overridden by a `--channel` run, so this is the channel the cron posts to. channel = CHANNEL def description(self) -> str: return "REO release regressions needing action posted to Slack" def all_include_fields(self) -> bool: - # The fields a search asks for are FIELDS and BURNDOWN_FIELDS and - # nothing else. `BzCleaner` would otherwise add `summary` to every - # query, which is the one field this message does not print. + # `BzCleaner` would otherwise add `summary` to every query. return True def has_default_products(self) -> bool: - # Both queries are scoped by classification, as bugdash's are; the - # default product list would report a different bug set. + # Scoped by classification instead, as bugdash's queries are. return False def filter_no_nag_keyword(self) -> bool: - # [no-nag] is a request not to mail a bug's assignee about it. This - # message names teams rather than people and is read by the release - # managers chasing the work, so dropping those bugs would hide work - # that still has to be done. + # [no-nag] is a request not to mail a bug's assignee. This message names + # teams rather than people and is read by the release managers chasing + # the work, so dropping those bugs would hide work still to be done. return False def add_custom_arguments(self, parser: argparse.ArgumentParser) -> None: @@ -683,25 +474,16 @@ def get_bz_params(self, date: str) -> BzParams: return self.params def bughandler(self, bug: Bug, data: dict[str, Any]) -> None: - """Keep every field of the bug, keyed by its id. - - `BzCleaner`'s own handler reduces a bug to the columns of an email - table, its summary included. This message ages every bug and counts it, - so it needs the fields it asked for and none of the rest. - """ + """Keep every field, where `BzCleaner` would keep the email columns.""" data[str(bug["id"])] = bug def fetch_bugs(self, query: dict, fields: str = FIELDS) -> list[dict]: """Run one of this rule's queries through `BzCleaner`'s search path. - Several queries per run -- one per version, plus one per burndown line - -- each one set here and read back by `get_bz_params`, the way - `warn_regressed_by` steps through its two. Going through `get_bugs` is - what attaches bugbot's API key, which is the whole reason this sees - restricted bugs, along with the query timeout and the paging. libmozdata - pages a search itself -- counting first, then walking the results in - chunks -- but only for a query carrying none of count_only, limit, order - or offset, so no query here may add one. + One query per version plus one per burndown line, each set here and read + back by `get_bz_params`, the way `warn_regressed_by` steps through its + two. libmozdata only pages a query carrying none of count_only, limit, + order or offset, so no query here may add one. """ self.params = {**query, "include_fields": fields} @@ -710,12 +492,8 @@ def fetch_bugs(self, query: dict, fields: str = FIELDS) -> list[dict]: def open_regressions(self, versions: dict[str, int]) -> list[dict]: """Every open release regression across the channels, each bug listed once. - A regression affecting Nightly usually affects Beta and Release too, so - the three queries overlap heavily: 62 hits covering 50 bugs when this - was written. Keying on the bug id merges them, which is the point of - this message — one list of what needs doing, not the same bug asked - about three times. Where two channels disagree the last query wins, but - the fields the buckets look at are all channel independent. + The three queries overlap heavily. Where they disagree the last one wins, + but the fields the buckets look at are all channel independent. """ bugs: dict[int, dict] = {} for version in sorted(set(versions.values())): @@ -729,14 +507,9 @@ def burndown_group( ) -> str: """Build the burndown bullet for one channel, aged from when each bug was fixed. - Unlike the other buckets this is per version rather than merged across the - channels: a fix reaches Beta and Release by separate uplifts, so the same bug - can be outstanding on one and done on the other, and each has to be asked for - against its own version. - - Nothing is subtracted for a bug fixed in the version's own cycle, as the - query only keeps bugs the version is still marked as affected by. Once a fix - is uplifted the status goes to fixed and the bug leaves the list. + Per version rather than merged across the channels: a fix reaches Beta and + Release by separate uplifts, so the same bug can be outstanding on one and + done on the other. """ query = burndown_query(version, utils.get_flag(None, "approval", channel)) bugs = [ @@ -749,33 +522,20 @@ def burndown_group( return stuck_group(bugs, label, "resolved") def post_message(self, blocks: list[dict]) -> None: - """Post the message to Slack, or print it when the run isn't for real. - - A dry run prints what it would have posted, so `--production` means here - what it means for every other rule. `test_mode` is honoured alongside it - for the reason `triage_owner_rotations` honours it: a test run must reach - nobody. - """ + """Post the message to Slack, or print it on a dry or test run.""" if self.dryrun or self.test_mode: print("DRY RUN: message not posted.\n") for block in blocks: print(block_text(block)) return - # HEADING is the message's notification fallback text, which is what a - # client that cannot render blocks shows instead of them. + # HEADING is the notification fallback text, which is what a client that + # cannot render blocks shows instead of them. slack.post_to_slack(self.channel, HEADING, blocks=blocks) logger.info("Rule %s posted to %s", self.name(), self.channel) def get_email_data(self, date: str) -> EmailData: - """Post the message, and give `send_email` nothing to send. - - The report is the Slack message rather than an email, and an empty list - is what stops one being sent -- the same way `security_affected_versions` - runs the pipeline for the needinfos it posts and mails no summary. The - "No data" line `send_email` then logs is about that email, not about the - message, which has been posted by the time it is written. - """ + """Post the message, and return no data so `send_email` sends nothing.""" self.post_message(self.blocks(versions_to_report())) return [] @@ -783,12 +543,8 @@ def get_email_data(self, date: str) -> EmailData: def blocks(self, versions: dict[str, int]) -> list[dict]: """Build the action required message, one section per bucket. - A header block titles the message and a context block labels it, then the - standing ask and each bucket that has anything in it follow as sections. - - The title is a header rather than bold text in a section so that it - renders at heading weight and separates the ask from the list. Header - blocks take plain text only, which is why nothing else lives in there. + Header blocks take plain text only, which is why the cadence is a separate + context block. """ titles: list[dict] = [ {"type": "header", "text": {"type": "plain_text", "text": HEADING}}, @@ -811,15 +567,8 @@ def blocks(self, versions: dict[str, int]) -> list[dict]: for channel in UPLIFT_CHANNELS: version = versions.get(channel) if version is None: - # A channel with no version to query. Skipped rather than raised, - # so adding a channel above can never be the thing that costs the - # whole message, and said out loud so it isn't a silent no-op - # either. - # - # ESR is the case that will turn up. `get_versions_from_trains` - # reports an esr version and `utils.get_flag` formats its numbered - # approval flag, so adding it here is now only a question of - # whether we want the line, not of whether the name can be built. + # Skipped rather than raised, so adding a channel above can never + # be the thing that costs the whole message. logger.warning("No version for %s; skipping its burndown line", channel) continue diff --git a/bugbot/slack.py b/bugbot/slack.py index f919d24de..6e0bd41ff 100644 --- a/bugbot/slack.py +++ b/bugbot/slack.py @@ -4,30 +4,12 @@ """Post messages to Slack. -One bot for the whole of bugbot. Everything about who is posting lives here -- -the token it authenticates with and the name it appears under -- and a caller -supplies only the message and where to send it. Where a rule posts is that -rule's to say, kept wherever the rest of its configuration is -- in the rule, or -in a module it shares with the other rules it posts alongside -- so a second -rule posting somewhere else needs no change in here. - -Messages go through chat.postMessage, which needs a bot token carrying -chat:write, and chat:write.public as well to post to a channel the bot has not -been invited to. One token serves every rule, so it is read from here rather -than passed in: it is a secret, and comes from `slack_bot_token` in -`configs/config.json` or from `SLACK_ACCESS_TOKEN`. - -A channel is an ID rather than a name -- a "C…" string, the last section of a -channel's 'copy link' URL, or "D…" for a DM. Unlike the token it is not a -secret, so it belongs with a rule's configuration and not in config.json. - -Messages are posted under `USERNAME` rather than whatever the Slack app happens -to be called, which needs `chat:write.customize` on the token as well. +One bot for the whole of bugbot: the token and the name it appears under live +here, and a caller supplies only the message and where to send it. `SLACK_API_URL`, `SLACK_ACCESS_TOKEN` and the error wording follow taskcluster's -notify service (services/notify), which solves the same problem. `SLACK_API_URL` -exists to point at a test server, which is the only way to exercise any of this -without a real token. +notify service (services/notify). `SLACK_API_URL` points at a test server, which +is the only way to exercise this without a real token. """ import json @@ -43,34 +25,22 @@ API_URL_VAR = "SLACK_API_URL" TOKEN_VAR = "SLACK_ACCESS_TOKEN" -# The key the bot token lives under in `configs/config.json`. Not required: a -# deployment that posts to no channel needs no token, so it is read with `.get` -# rather than validated at load time the way `bz_api_key` is. +# Read with `.get` rather than validated at load time the way `bz_api_key` is: a +# deployment that posts to no channel needs no token. TOKEN_KEY = "slack_bot_token" -# The name every message is posted under. Not overridable: there is one bot, so -# there is one name, and a rule choosing its own would only make bugbot look like -# several senders. -# -# A Slack app's own name is set in its app configuration, is shared by everything -# the token posts, and is generally not what a reader of one of these messages -# should see. This is what they see instead. -# -# Sending it needs `chat:write.customize` on the token on top of `chat:write`. -# Slack rejects the message outright when that scope is missing rather than -# ignoring the name, so this is not something that quietly stops working. +# The name every message is posted under, instead of whatever the Slack app +# happens to be called. Needs `chat:write.customize` on the token, and Slack +# rejects the message outright when that scope is missing. USERNAME = "Firefox Release Management Bot" def get_token() -> str: - """The bot token to post with. + """The bot token to post with, `SLACK_ACCESS_TOKEN` winning over the config. - `SLACK_ACCESS_TOKEN` wins over `slack_bot_token` in `configs/config.json`. A - missing config file counts as a missing key rather than an error, so a - checkout with no credentials still imports. - - Raises rather than returning empty: these are cron jobs whose whole purpose is - the message, so a missing token has to stop the run and be seen. + A missing config file counts as a missing key, so a checkout with no + credentials still imports. Raises rather than returning empty: these are cron + jobs whose whole purpose is the message. """ token = os.environ.get(TOKEN_VAR, "").strip() if token: @@ -99,28 +69,19 @@ def post_to_slack( ) -> str: """Post a message to a Slack channel, and return its timestamp. - `channel` is a channel ID; see the module docstring. Who the message comes - from is not a caller's concern: the token and the display name are this - module's, and every rule posts as the same bot. - - `text` is always sent: on a blocks message it is the notification and the - fallback for clients that can't render blocks. - - `thread_ts` replies in thread, and takes the timestamp this returns for an - earlier message. - - Link previews are always suppressed. These messages are notifications built - around their links, and an unfurl below one repeats what the message already - says at several times the height. + `channel` is a channel ID, the last section of a channel's 'copy link' URL. + `text` is the notification and the fallback for clients that can't render + blocks. `thread_ts` takes the timestamp this returns for an earlier message. Not retried, unlike reads: a POST that times out may well have arrived, so - retrying risks posting the message twice. A failure here fails the run - instead, which is visible in the error digest and harmless to repeat by hand. + retrying risks posting the message twice. """ payload: dict = { "channel": channel, "text": text, "username": USERNAME, + # These messages are built around their links, and an unfurl below one + # repeats what the message already says. "unfurl_links": False, "unfurl_media": False, } @@ -131,9 +92,8 @@ def post_to_slack( api_url = (os.environ.get(API_URL_VAR) or DEFAULT_API_URL).rstrip("/") - # The body is encoded here rather than passed as `json=` so the charset can be - # spelled out: Slack answers a bare application/json with a missing_charset - # warning. + # Encoded here rather than passed as `json=` so the charset can be spelled + # out: Slack answers a bare application/json with a missing_charset warning. response = requests.post( f"{api_url}/chat.postMessage", data=json.dumps(payload).encode("utf-8"), @@ -148,8 +108,7 @@ def post_to_slack( f"Slack returned HTTP {response.status_code}: {response.text.strip()}" ) - # chat.postMessage reports application errors as HTTP 200 with ok=false, so the - # body is what has to be checked rather than the status. + # chat.postMessage reports application errors as HTTP 200 with ok=false. result = response.json() if not result.get("ok"): reason = result.get("error", result)