Skip to content

Support the translation tests better outside of CI#51733

Merged
kevingranade merged 3 commits intoCleverRaven:masterfrom
jbytheway:translation-tests
Sep 24, 2021
Merged

Support the translation tests better outside of CI#51733
kevingranade merged 3 commits intoCleverRaven:masterfrom
jbytheway:translation-tests

Conversation

@jbytheway
Copy link
Copy Markdown
Contributor

@jbytheway jbytheway commented Sep 20, 2021

Summary

None

Purpose of change

The new translation tests require a .mo file to be compiled related to the TEST_DATA mod. This was done for CI in the build.sh script, but that's not appropriate for local users.

Describe the solution

Move the commands from build.sh to Makefile.

Describe alternatives you've considered

Running the build directly from within the unit test.

Testing

Works OK locally.

This is probably going to break the CMake CI, so opening as draft to see what happens.

Additional context

@BrettDong Based on my local testing it seems like the tests only require the one TEST_DATA .mo file; you don't have to run compile_mo.sh; is that right?

@BrettDong
Copy link
Copy Markdown
Member

BrettDong commented Sep 20, 2021

Both real production translations (./lang/compile_mo.sh) and test translation (TEST_DATA) need to be compiled.

@actual-nh actual-nh added <Bugfix> This is a fix for a bug (or closes open issue) Code: Tests Measurement, self-control, statistics, balancing. Translation I18n Code: Build Issues regarding different builds and build environments labels Sep 20, 2021
@BrettDong
Copy link
Copy Markdown
Member

BrettDong commented Sep 20, 2021

Test translations should be compiled only for tests but not for release distribution, so $(TEST_MO) should be a dependency on tests (only when LOCALIZE is enabled) but not on localization.

BTW what is check target? I can't find any reference to the target in Makefile and build.sh.

@jbytheway
Copy link
Copy Markdown
Contributor Author

jbytheway commented Sep 20, 2021

BTW what is check target? I can't find any reference to the target in Makefile and build.sh.

It isn't used anywhere in the build script, it's the standard way to run the tests locally. See e.g. the GNU standard targets.

@BrettDong
Copy link
Copy Markdown
Member

BrettDong commented Sep 20, 2021

Compiling mo in the Makefile path in build.sh at around line 210 can also be removed?

@jbytheway
Copy link
Copy Markdown
Contributor Author

jbytheway commented Sep 20, 2021

While I'm cleaning things up, do we still need the English-related special cases in compile_mo.sh?

@BrettDong
Copy link
Copy Markdown
Member

BrettDong commented Sep 20, 2021

English .mo is no longer necessary and can be safely deleted.

@jbytheway jbytheway force-pushed the translation-tests branch 2 times, most recently from 0821656 to b7f1eeb Compare September 20, 2021 15:27
@actual-nh
Copy link
Copy Markdown
Contributor

actual-nh commented Sep 20, 2021

Are there any of the "cleans" that should be removing lang/mo_built.stamp along with TEST_MO?

@jbytheway jbytheway changed the title Translation tests Support the translation tests better outside of CI Sep 20, 2021
@jbytheway
Copy link
Copy Markdown
Contributor Author

jbytheway commented Sep 20, 2021

lang/mo_built.stamp

Good catch. Added.

@jbytheway
Show path in tests that check for path
252341a 
Some of the translation tests verify the existence of a path, but don't
say what that path is on failure.

Capture the path so that they do.
@jbytheway
Remove English special-case code in compile_mo.sh
af08c0d 
This related to a libintl bug and is no longer relevant now that we
don't use libintl.
@jbytheway
Compile test .mo in Makefile and CMake
ff4db05 
The new translation tests require a .mo file to be compiled related to
the TEST_DATA mod.  This was done for CI in the build.sh script, but
that's not appropriate for local users.

Move the commands from build.sh to Makefile and CMakeLists.txt.

Also, add a stamp file so that the Makefile does not rebuild the .mo
files unnecessarily.
@jbytheway jbytheway marked this pull request as ready for review September 24, 2021 01:31
@jbytheway
Copy link
Copy Markdown
Contributor Author

jbytheway commented Sep 24, 2021

CMake build passed this time, so marking ready for review.

BrettDong
BrettDong approved these changes Sep 24, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@BrettDong BrettDong left a comment

Choose a reason for hiding this comment

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

Thank you!

@kevingranade kevingranade merged commit fb48f34 into CleverRaven:master Sep 24, 2021
@jbytheway jbytheway deleted the translation-tests branch September 24, 2021 11:41
@BrettDong BrettDong mentioned this pull request Sep 24, 2021
Merged
@olanti-p olanti-p mentioned this pull request Sep 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BrettDong BrettDong BrettDong approved these changes

Assignees

No one assigned

Labels

<Bugfix> This is a fix for a bug (or closes open issue) Code: Build Issues regarding different builds and build environments Code: Tests Measurement, self-control, statistics, balancing. Translation I18n

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jbytheway @BrettDong @actual-nh @kevingranade