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

Merged
merged 8 commits into from
Nov 25, 2024
Merged

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

merged 8 commits into from
Nov 25, 2024

Conversation

cfm
Copy link
Member

@cfm cfm commented Oct 4, 2024 •
edited by legoktm
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

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
Closed
@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 cfm force-pushed the l10n-ci-screenshots branch 5 times, most recently from 0cdda11 to 7900e21 Compare October 11, 2024 23:45
@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 :/

@cfm cfm assigned cfm and unassigned legoktm Nov 20, 2024
@cfm
chore: un-special-case en_US as a supported language
63fdf2e 
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
5f13d68 
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…
229ca32 
…s sent in the "Accept-Languages" header
@cfm
Copy link
Member Author

cfm commented Nov 20, 2024

The good news is that the screenshots are being captured correctly. The bad news is that the tests themselves still aren't (and haven't been) parameterized fully with the intended locale. I'm looking into it.

@cfm
fix: configure SUPPORTED_LOCALES=$TEST_LOCALES in all fixtures
1967f1d 
Otherwise they provide only the default SUPPORTED_LOCALES =
[FALLBACK_LOCALE] = ["en_US"].
@cfm
fix(firefox_web_driver): configure Firefox's locale at fixture time
3179fd4 
Otherwise the fixture isn't set up to request the language expected by
the tests that use it.
@cfm
Copy link
Member Author

cfm commented Nov 21, 2024

I found a couple more fundamental parameterization problems yesterday and today, for which I've just force-pushed fixes. I'm going to monitor translation-tests for failing string checks before this is ready for another review.

@cfm
refactor: move locale parameterization from tests to the firefox_web_…
e96469b 
…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 cfm force-pushed the l10n-ci-screenshots branch 2 times, most recently from 0d31f8c to 550e0f3 Compare November 22, 2024 00:03
@legoktm
Copy link
Member

legoktm commented Nov 22, 2024

I'd be happy to approve parts of this in separate PRs if that would be easier for you. e.g. the saving as artifacts part is totally fine and good to go, same with some of the other intermediate fixes.

@cfm
fix(_create_firefox_driver): set "intl.accept_languages" via FirefoxO…
a5dcbd7 
…ptions rather than FirefoxProfile

<https://stackoverflow.com/a/71766991> claims that FirefoxProfile is in
fact deprecated as of Selenium 4.  I can't find any obvious evidence of
that, but it works.
@cfm
test: don't search for source strings when testing a translation
6c9917c 
And run all Journalist Interface page-layout tests in Firefox.
@cfm
Copy link
Member Author

cfm commented Nov 22, 2024

Thanks, @legoktm. As of 6c9917c I've now got this sorted for the Journalist Interface in Firefox. Tomorrow I should be able to fix up the Source Interface in Tor Browser the same way. If not, I'll split it between the two interfaces.

@cfm
Copy link
Member Author

cfm commented Nov 25, 2024

Ready here for the Journalist Interface. The Source Interface I've deferred to #7354.

@cfm cfm removed their assignment Nov 25, 2024
@legoktm legoktm self-assigned this Nov 25, 2024
legoktm
legoktm approved these changes Nov 25, 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.

Awesome, JI side looks good; thanks!

@legoktm legoktm added this pull request to the merge queue Nov 25, 2024
Merged via the queue into develop with commit 802c8da Nov 25, 2024
67 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@legoktm legoktm legoktm approved these changes

Assignees

@legoktm legoktm

Labels
goals: sick CI i18n Anything related to translation or internationalization of SecureDrop
Projects
SecureDrop dev cycle
Status: Done
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