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

ci(translation): save screenshots as workflow artifacts #7240

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

ci(translation): save screenshots as workflow artifacts #7240

wants to merge 7 commits into from

Conversation

cfm
Copy link
Member

@cfm cfm commented Oct 4, 2024 •
edited
Loading

Status

Ready for review

Description of Changes

Fixes #7237 by saving the screenshots generated by make translation-test as CI artifacts for 24 hours, for the following reasons:

  1. make translation-test generates them no matter what, so we might as well make them inspectable.
  2. generation of screenshots is broken #7237 occurs only locally, where this code path is rarely used; let's take advantage of CI exercising it.
  3. This points towards perhaps even updating Weblate's copy of screenshots directly from these tests run nightly on develop.

Fixes #7241 by correcting the parameterization of tests and fixtures for page-layout tests. See individual commits for details.

Testing

  • Visual review.
  • Inspect a couple of languages and confirm that their screenshots are correct.
  • 24 hours is a reasonable retention period for 100 MB of screenshots per run.

Deployment

CI-only; no deployment considerations.

@cfm cfm added i18n Anything related to translation or internationalization of SecureDrop goals: sick CI labels Oct 4, 2024
@cfm cfm force-pushed the l10n-ci-screenshots branch 2 times, most recently from 3fe8933 to e844085 Compare October 4, 2024 23:31
@cfm cfm marked this pull request as ready for review October 4, 2024 23:31
@cfm cfm requested a review from a team as a code owner October 4, 2024 23:31
@cfm cfm force-pushed the l10n-ci-screenshots branch 2 times, most recently from be676ea to 9191b91 Compare October 5, 2024 04:36
@legoktm legoktm self-assigned this Oct 7, 2024
legoktm
legoktm requested changes Oct 7, 2024
View reviewed changes
Copy link
Member

@legoktm legoktm left a comment

Choose a reason for hiding this comment

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

See inline comment.

I retriggered CI so there are artifacts that can be downloaded from https://github.com/freedomofpress/securedrop/actions/runs/11190459190 - but when I looked at some of the el and de_DE screenshots, they were all in English. (Or did I just happen to look at the wrong screenshots?)

.github/workflows/translation.yml Outdated Show resolved Hide resolved
@cfm cfm mentioned this pull request Oct 7, 2024
Open
@cfm
Copy link
Member Author

cfm commented Oct 7, 2024

Good catch, @legoktm. I'm looking into this in #7241.

@cfm cfm force-pushed the l10n-ci-screenshots branch 2 times, most recently from 6d4712a to 7edff79 Compare October 7, 2024 23:53
@cfm
Copy link
Member Author

cfm commented Oct 7, 2024

The fixes are in as of #7241 (comment). Two cheers for putting rarely-exercised code paths in CI! Back to you, @legoktm.

@cfm cfm requested a review from legoktm October 7, 2024 23:57
@cfm
chore: un-special-case en_US as a supported language
4adf0da 
Only languages listed in "i18n.json" are reported by "make
supported-languages", included in the CI matrix, and usable as $LANG in
"LOCALES=$LANG make test".
@cfm
ci: save translation screenshots as workflow artifacts
406fe22 
This is about 4 MB × n languages, currently < 100 MB per run.
@cfm
test(got_expected_language): html[lang] should be one of the language…
c870c83 
…s sent in the "Accept-Languages" header
@cfm
fix(sd_servers): configure SUPPORTED_LOCALES=$TEST_LOCALES
62d1934 
Otherwise this fixture provides only the default SUPPORTED_LOCALES =
[FALLBACK_LOCALE] = ["en_US"].
@cfm
fix(firefox_web_driver): configure Firefox's locale at fixture time
3e740c1 
Otherwise the fixture isn't set up to request the language expected by
the tests that use it.
@cfm
test(test_ossec_alert_button): don't search for source string in tran…
f8ecf5e 
…slation
@cfm cfm force-pushed the l10n-ci-screenshots branch 2 times, most recently from 2689288 to 0cdda11 Compare October 11, 2024 23:08
@cfm
refactor: move locale parameterization from tests to the firefox_web_…
7900e21 
…driver fixture

That is, instead of receiving a "locale" fixture directly, tests that
use the "firefox_web_driver" fixture are now parameterized indirectly,
via the firefox_web_driver.locale attribute received from the
parameterized fixture.
@cfm
Copy link
Member Author

cfm commented Oct 11, 2024 •
edited
Loading

I wrestled with this some more today and found another problem with how the firefox_web_driver fixture was parameterized, which I've refactored in 7900e21. Back to you for real, @legoktm.

@legoktm
Copy link
Member

legoktm commented Oct 16, 2024

When I download screenshots-de_DE from https://github.com/freedomofpress/securedrop/actions/runs/11300734349, I see three subfolders, de-DE, de_DE, and en-US. So somewhere the dash is being normalized differently?

And...the de screenshots are still in English :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legoktm legoktm Awaiting requested review from legoktm

Requested changes must be addressed to merge this pull request.

Assignees

@legoktm legoktm

Labels
goals: sick CI i18n Anything related to translation or internationalization of SecureDrop
Projects
SecureDrop dev cycle
Status: Under Review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

screenshots, and maybe translation tests, run only under en_US generation of screenshots is broken
2 participants
@cfm @legoktm