Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test coverage for product-details rebuild #1595

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Add test coverage for product-details rebuild #1595

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f22ca26
product-details: move git clone / push to separate functions for easi…
jcristau Dec 17, 2024
02e62e7
tests: add coverage for product-details rebuild
jcristau Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions api/src/shipit_api/admin/product_details.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,20 +1106,7 @@ def run_check(*arg, **kw):
return cli_common.utils.retry(lambda: cli_common.command.run_check(*arg, **kw))


async def rebuild(
db_session: sqlalchemy.orm.Session,
git_branch: str,
git_repo_url: str,
folder_in_repo: str,
breakpoint_version: typing.Optional[int],
clean_working_copy: bool = True,
):
secrets = [urllib.parse.urlparse(git_repo_url).password]

# Sometimes we want to work from a clean working copy
if clean_working_copy and shipit_api.common.config.PRODUCT_DETAILS_DIR.exists():
shutil.rmtree(shipit_api.common.config.PRODUCT_DETAILS_DIR)

def setup_working_copy(git_branch, git_repo_url, secrets):
# Clone/pull latest product details
logger.info(f"Getting latest product details from {cli_common.command.hide_secrets(git_repo_url, secrets)}.")
if shipit_api.common.config.PRODUCT_DETAILS_DIR.exists():
Expand All @@ -1141,6 +1128,23 @@ async def rebuild(
run_check(["git", "config", "user.email", "[email protected]"], cwd=shipit_api.common.config.PRODUCT_DETAILS_DIR, secrets=secrets)
run_check(["git", "config", "user.name", "Release Services Robot"], cwd=shipit_api.common.config.PRODUCT_DETAILS_DIR, secrets=secrets)


async def rebuild(
db_session: sqlalchemy.orm.Session,
git_branch: str,
git_repo_url: str,
folder_in_repo: str,
breakpoint_version: typing.Optional[int],
clean_working_copy: bool = True,
):
secrets = [urllib.parse.urlparse(git_repo_url).password]

# Sometimes we want to work from a clean working copy
if clean_working_copy and shipit_api.common.config.PRODUCT_DETAILS_DIR.exists():
shutil.rmtree(shipit_api.common.config.PRODUCT_DETAILS_DIR)

setup_working_copy(git_branch, git_repo_url, secrets)

# XXX: we need to implement how to figure out breakpoint_version from old_product_details
# if breakpoint_version is not provided we should figure it out from old_product_details
# and if we can not figure it out we should use shipit_api.common.config.BREAKPOINT_VERSION
Expand Down Expand Up @@ -1303,4 +1307,8 @@ async def rebuild(
# XXX: we need a better commmit message, maybe mention what triggered this update
commit_message = "Updating product details"
run_check(["git", "commit", "-m", commit_message], cwd=shipit_api.common.config.PRODUCT_DETAILS_DIR, secrets=secrets)
run_check(["git", "push", "origin", git_branch], cwd=shipit_api.common.config.PRODUCT_DETAILS_DIR, secrets=secrets)
git_push(git_branch, secrets)


def git_push(git_branch, secrets):
run_check(["git", "push", "origin", git_branch], cwd=shipit_api.common.config.PRODUCT_DETAILS_DIR, secrets=secrets)
97 changes: 95 additions & 2 deletions api/tests/test_product_details.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,33 @@
# 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 pathlib
import re
import subprocess
from unittest import mock

import aiohttp
import pytest
from aioresponses import aioresponses
from sqlalchemy import engine, event

import shipit_api.admin.product_details
from shipit_api.admin.product_details import fetch_l10n_data
from shipit_api.common.models import Release
from shipit_api.admin.product_details import fetch_l10n_data, rebuild
from shipit_api.common.models import Release, Version


# product_details uses a postgresql-specific "split_part" sql function
@event.listens_for(engine.Engine, "connect")
def setup_split_part(dbapi_connection, conn_rec):
def split_part(string, delimiter, position):
return string.split(delimiter, position)[position - 1]

dbapi_connection.create_function(
"split_part",
3,
split_part,
)


def create_html(folder, items):
Expand Down Expand Up @@ -72,3 +89,79 @@ async def test_fetch_l10n_data():
m.get(url, status=200, payload=dict(a="a"))
(_, changesets) = await fetch_l10n_data(session, release, raise_on_failure=True, use_cache=False)
assert changesets == {"a": "a"}


def mock_setup_working_copy(branch, url, secrets):
subprocess.check_call(["git", "clone", "-n", url, str(shipit_api.common.config.PRODUCT_DETAILS_DIR)])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not great, the test should be self-contained, but it was easiest for now. There's a few other network requests done as part of the rebuild that should be mocked as well...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ideal, but anything is better than nothing at this point...

Copy link
Member

@gabrielBusta gabrielBusta Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there could be a static data directory under tests? Then you could store a snapshot of product details data in there and init a git repo at test run-time. I think at that point you wouldn't need to mock the function (can probably use the local repo path as the url)

The raw data is about 32M:

git clone https://github.com/mozilla-releng/product-details.git
Cloning into 'product-details'...
... done.
du -sh product-details
 44M	product-details
rm -rf product-details/.git
du -sh product-details
 32M	product-details

Maybe it can even be an empty repo...

The network calls to HG for the l10n data are more likely to fail/hit rate limits. Those would be good to mock.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll land this before I run out of steam, and try and improve later. We can probably get away with a lot less than 32M (maybe even git init an empty repo) and still get meaningful testing, but I'd rather not block on that given you two already reviewed this.

subprocess.check_call(["git", "checkout", "-b", branch, "906b7cd284728a2acec695ddcba9193b44d38982"], cwd=shipit_api.common.config.PRODUCT_DETAILS_DIR)
subprocess.check_call(["git", "config", "user.email", "[email protected]"], cwd=shipit_api.common.config.PRODUCT_DETAILS_DIR)
subprocess.check_call(["git", "config", "user.name", "Release Services Robot"], cwd=shipit_api.common.config.PRODUCT_DETAILS_DIR)


def mock_git_push(branch, secrets):
return


@pytest.mark.asyncio
@mock.patch("shipit_api.admin.product_details.setup_working_copy", mock_setup_working_copy)
@mock.patch("shipit_api.admin.product_details.git_push", mock_git_push)
async def test_rebuild(app, tmp_path):
fxnightly = Version(product_name="firefox", current_version="135.0a1", product_channel="nightly")
tbnightly = Version(product_name="thunderbird", current_version="135.0a1", product_channel="nightly")
deved = Release(
product="devedition",
branch="releases/mozilla-beta",
version="134.0b9",
revision="615791f9752c70ef1757abf68544c8275f219ce3",
build_number=1,
release_eta=None,
status="shipped",
partial_updates=None,
)
beta = Release(
product="firefox",
branch="releases/mozilla-beta",
version="134.0b8",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this + having a newer DevEdition version should cover the issue that caused the original backout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I verified the test is failing without the "don't skip l10n for devedition entirely" commit with the "Too few firefox primary builds for FIREFOX_DEVEDITION" error.

revision="9fb87e89c26069198ce2a59a0a790a264d225169",
build_number=1,
release_eta=None,
status="shipped",
partial_updates=None,
)
release = Release(
product="firefox",
branch="releases/mozilla-release",
version="133.0",
revision="8141aab3ba856d7cbae6c851dd71f2e0cb69649c",
build_number=2,
release_eta=None,
status="shipped",
partial_updates=None,
)
app.db.session.add(fxnightly)
app.db.session.add(tbnightly)
app.db.session.add(deved)
app.db.session.add(beta)
app.db.session.add(release)
app.db.session.commit()
with (
mock.patch("shipit_api.common.config.PRODUCT_DETAILS_DIR", tmp_path / "product-details"),
mock.patch("shipit_api.common.config.PRODUCT_DETAILS_NEW_DIR", tmp_path / "product-details-new"),
mock.patch("shipit_api.common.config.PRODUCT_DETAILS_CACHE_DIR", tmp_path / "product-details-cache"),
):
await rebuild(app.db.session, "testing", "https://github.com/mozilla-releng/product-details", "public", 130)

parent = tmp_path / "product-details" / "public" / "1.0"
with (parent / "firefox_versions.json").open() as f:
versions = json.load(f)
assert versions["FIREFOX_NIGHTLY"] == "135.0a1"
assert versions["FIREFOX_DEVEDITION"] == "134.0b9"
assert versions["LATEST_FIREFOX_DEVEL_VERSION"] == "134.0b8"
assert versions["LATEST_FIREFOX_VERSION"] == "133.0"

with (parent / "firefox_primary_builds.json").open() as f:
primary_builds = json.load(f)
assert set(primary_builds["ach"].keys()) == {"133.0", "134.0b8", "134.0b9", "135.0a1"}

assert not list(parent.glob("l10n/Devedition-*"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and this covers #1596, I assume?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

assert next(parent.glob("l10n/Firefox-134.0b8-*")).name == "Firefox-134.0b8-build1.json"