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

Jump to bottom

Bug 1636842 - part 2: Make check-screenshots script smarter about the number of screenshots #6689

Merged
merged 1 commit into from
May 27, 2020
Merged

Bug 1636842 - part 2: Make check-screenshots script smarter about the number of screenshots #6689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
95 changes: 95 additions & 0 deletions l10n-screenshots-config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
locales:
- af
- an
- anp
- ar
- ast
- az
- bg
- bn
- bo
- br
- bs
- ca
- co
- cs
- cy
- da
- de
- dsb
- el
- en-CA
- en-GB
- en-US
- eo
- es-AR
- es-CL
- es-MX
- es
- eu
- fa
- fi
- fr
- ga-IE
- gd
- gl
- gu-IN
- he
- hi-IN
- hr
- hsb
- hu
- hy-AM
- ia
- id
- is
- it
- ja
- jv
- ka
- kab
- kk
- km
- kn
- ko
- lo
- lt
- lv
- ml
- mr
- ms
- my
- nb-NO
- ne-NP
- nl
- nn-NO
- oc
- or
- pa-IN
- pl
- pt-BR
- pt-PT
- rm
- ro
- ru
- ses
- si
- sk
- sl
- sq
- su
- sv-SE
- ta
- te
- th
- tl
- tr
- uk
- ur
- uz
- vi
- zgh
- zh-CN
- zh-TW
locales-with-shorter-context-menu: ["hi-IN", "th", "zh-CN", "zh-TW"] # These locales have 1 less screenshot.
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 capturing edge cases by defining a list of locales that behave the same way is more maintainable. I thought of making a file that tells how many screenshots each single locale has, but that'll be cumbersome and prone to error to update.

usual-number-of-screenshots: 55
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ If we add/remove tests, then we must update this value.

92 changes: 0 additions & 92 deletions l10n-screenshots-locales.txt
View file
Open in desktop

This file was deleted.

11 changes: 6 additions & 5 deletions l10n-screenshots.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ PROJECT_DIR="$(get_abs_path $CURRENT_DIR/../../../..)"


if [ -d l10n-screenshots ]; then
echo "The l10n-screenshots directory already exists. You decide."
exit 1
echo "The l10n-screenshots directory already exists. You decide."
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space? also above and some lines below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I aligned every if statement to use 4-space-indents. The rest of the file seems to use this convention. I'm fine having everything aligned at 2 spaces too 🙂

fi

if [ ! -d firefoxios-l10n ]; then
Expand All @@ -25,13 +25,14 @@ fi
mkdir -p l10n-screenshots

if [ "$1" = '--test-without-building' ]; then
EXTRA_FAST_LANE_ARGS='--test_without_building'
shift
EXTRA_FAST_LANE_ARGS='--test_without_building'
shift
fi

LOCALES=$*
if [ $# -eq 0 ]; then
LOCALES=$(cat "$PROJECT_DIR/l10n-screenshots-locales.txt")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not easy to parse a Yaml file in bash and I don't think we want to run all locales by default anymore. So I updated the script to force users to tell which locales they want to run. How does this sound to you @isabelrios?

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good, that would only affect when running them locally, right? on CI, this is passed each time if I got this right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right!

echo "Please provide locales to test. Available locales live in 'l10n-screenshots-config.yml'. E.g.: $0 af an anp ar"
exit 1
fi

for lang in $LOCALES; do
Expand Down
1 change: 1 addition & 0 deletions taskcluster/docker/screenshots/requirements.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
aiohttp-retry
PyYAML
taskcluster
7 changes: 3 additions & 4 deletions taskcluster/ffios_taskgraph/screenshots_locales.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@ def get_screenshots_locales():
current_dir = os.path.dirname(os.path.realpath(__file__))
project_dir = os.path.realpath(os.path.join(current_dir, '..', '..'))

with open(os.path.join(project_dir, 'l10n-screenshots-locales.txt')) as f:
lines = f.readlines()
with open(os.path.join(project_dir, 'l10n-screenshots-config.yml')) as f:
config = yaml.safe_load(f)

locales = [line.strip() for line in lines]
return locales
return config["locales"]
5 changes: 1 addition & 4 deletions taskcluster/ffios_taskgraph/transforms/bitrise.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,6 @@ def add_bitrise_command(config, tasks):
yield task


_EXPECTED_NUMBER_OF_SCREENSHOTS_PER_LOCALE = 54


@transforms.add
def add_screenshot_checks_command(config, tasks):
for task in tasks:
Expand All @@ -114,7 +111,7 @@ def add_screenshot_checks_command(config, tasks):
"python3",
"taskcluster/scripts/check-screenshots.py",
"--artifacts-directory", _ARTIFACTS_DIRECTORY,
"--screenshots-per-locale", str(_EXPECTED_NUMBER_OF_SCREENSHOTS_PER_LOCALE),
"--screenshots-configuration", "l10n-screenshots-config.yml",
]

for locale in task["attributes"]["chunk_locales"]:
Expand Down
43 changes: 32 additions & 11 deletions taskcluster/scripts/check-screenshots.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import logging
import os
import sys
import yaml

from zipfile import ZipFile

Expand All @@ -21,12 +22,13 @@ def main():
parser = argparse.ArgumentParser(description="Ensures a directory has all expected locales and screenshots.")

parser.add_argument("--artifacts-directory", required=True, help="The directory containing the zip archives to look into")
parser.add_argument("--screenshots-per-locale", type=int, required=True, help="Number of expected screenshots for each locale")
parser.add_argument("--screenshots-configuration", type=argparse.FileType("r"), required=True, help="YAML file that contains the number of screenshots to expect as well as exceptions/")
parser.add_argument("--locale", dest="locales", metavar="LOCALE", action="append", required=True, help="locale that must be present(can be repeated)")

result = parser.parse_args()

_check_files(result.artifacts_directory, result.locales, result.screenshots_per_locale)
number_of_screenshots_per_locale = _parse_screenshots_configuration(result.screenshots_configuration)
_check_files(result.artifacts_directory, result.locales, number_of_screenshots_per_locale)


def _init_logging():
Expand All @@ -36,37 +38,56 @@ def _init_logging():
)


def _parse_screenshots_configuration(config_file):
config = yaml.safe_load(config_file)

number_of_screenshots_per_locale = {}
for locale in config["locales"]:
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 loop is in charge of counting how many screenshots a locale has. As of now a locale can have either 55 or 54 pictures. We may have other kinds of locales that bump/reduce this number. Each new kind will have to be defined here.

expected_screenshots = config["usual-number-of-screenshots"]
if locale in config["locales-with-shorter-context-menu"]:
expected_screenshots -= 1

number_of_screenshots_per_locale[locale] = expected_screenshots

return number_of_screenshots_per_locale


# This exit code was picked to avoid collision with famous exit codes (like 1 or 2). This way,
# Taskcluster can spot if this script failed for a known reason and rerun this task if needed.
_FAILURE_EXIT_CODE = 47


def _check_files(artifacts_directory, locales, expected_number_of_screenshots_per_locale):
def _check_files(artifacts_directory, locales, number_of_screenshots_per_locale):
errors = []

archives = set(glob.glob("{}/*.zip".format(artifacts_directory)))
archives = _filter_out_derived_data_archive(archives)
expected_archives = set("{}/{}.zip".format(artifacts_directory, locale) for locale in locales)
expected_number_of_screenshots_per_archive = {
"{}/{}.zip".format(artifacts_directory, locale): number_of_screenshots_per_locale[locale]
for locale in locales
}

expected_archives = set(expected_number_of_screenshots_per_archive.keys())
if archives != expected_archives:
errors.append(
"The list of archives (zip files) does not match the expected one. Expected: {}. Got: {}.".format(
expected_archives, archives
)
)

log.info("Processing {} archives...".format(len(archives)))
log.info("Processing {} archives...".format(len(expected_archives)))
log.debug("Archives are expected to have these numbers of screenshots: {}".format(expected_number_of_screenshots_per_archive))
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 way we know what the script expects.


for archive in sorted(archives): # Sorted archives enables sorted error messages
errors.extend(_check_single_archive(archive, expected_number_of_screenshots_per_locale))
for archive, expected_number_of_screenshots in expected_number_of_screenshots_per_archive.items():
errors.extend(_check_single_archive(archive, expected_number_of_screenshots))

if errors:
error_list = "\n * ".join(errors)
log.critical("Got {} error(s) while verifying screenshots: \n * {}".format(len(errors), error_list))
# TODO Uncomment the next line once screenshot tests are fixed on all locales.
# sys.exit(_FAILURE_EXIT_CODE)

log.info("No archive is missing and all of them contain the right number of screenshots")
log.info("No archive is missing and all of them contain the right number of screenshots!")


def _filter_out_derived_data_archive(archives):
Expand All @@ -82,7 +103,7 @@ def _filter_out_derived_data_archive(archives):
return filtered_out_archives


def _check_single_archive(archive, expected_number_of_screenshots_per_locale):
def _check_single_archive(archive, expected_number_of_screenshots):
errors = []

with ZipFile(archive) as zip_file:
Expand All @@ -93,10 +114,10 @@ def _check_single_archive(archive, expected_number_of_screenshots_per_locale):
errors.append('Archive "{}" contains non-png files: {}'.format(archive, non_png_files))

actual_number_of_screenshots = len(png_files)
if actual_number_of_screenshots != expected_number_of_screenshots_per_locale:
if actual_number_of_screenshots != expected_number_of_screenshots:
errors.append(
'Archive "{}" does not contain the expected number of screenshots. Expected: {}. Got: {}'.format(
archive, expected_number_of_screenshots_per_locale, actual_number_of_screenshots
archive, expected_number_of_screenshots, actual_number_of_screenshots
)
)

Expand Down