Make ECMPoQmToolsTest actually fail if a translation is wrong
ClosedPublic

Authored by wbauer on Jul 14 2017, 1:35 PM.

Details

Summary

Currently the test still passes even if one of the check_translations() calls fail.
The reason is that check_translations() is a function which has its own scope, so if it sets "failed" the upper level doesn't see it.

Use the PARENT_SCOPE option when setting the variable to properly propagate the value to the upper level.

Test Plan

Make a change so that the test is expected to fail, e.g. modify an expected output in tests/ECMPoQmToolsTest/check.cmake.in.

make test now will correctly mark the test as failed, before the test still passed.

Diff Detail

Repository
R240 Extra CMake Modules
Lint
Lint Skipped
Unit
Unit Tests Skipped
wbauer created this revision.Jul 14 2017, 1:35 PM
Restricted Application added projects: Frameworks, Build System. · View Herald TranscriptJul 14 2017, 1:35 PM
Restricted Application added subscribers: Build System, Frameworks. · View Herald Transcript

From the cmake documentation:

set(<variable> <value>... [PARENT_SCOPE])

Set the given `<variable>` in the current function or directory scope.

If the `PARENT_SCOPE` option is given the variable will be set in
the scope above the current scope. Each new directory or function
creates a new scope. This command will set the value of a variable
into the parent directory or calling function (whichever is applicable
to the case at hand).

wbauer updated this revision to Diff 16705.Jul 14 2017, 1:46 PM

New diff with full context.

aacid added a subscriber: apol.Jul 29 2017, 9:59 AM
apol added inline comments.Jul 30 2017, 10:48 PM
tests/ECMPoQmToolsTest/check.cmake.in
79

It should also change the other set(fail) calls.

Or am I missing something?

wbauer added inline comments.Jul 31 2017, 5:36 AM
tests/ECMPoQmToolsTest/check.cmake.in
79

The other set(fail) calls *are* at the top-level and not inside a function.
So, no, it doesn't make sense there.

The function only goes from line#69 to #84.

apol accepted this revision.Jul 31 2017, 10:50 AM

Right, thanks. Your explanation + coffee helped.

Maybe a next iteration after merging this moving the function out of the if would be useful.

This revision is now accepted and ready to land.Jul 31 2017, 10:50 AM
This revision was automatically updated to reflect the committed changes.
In D6701#130417, @apol wrote:

Maybe a next iteration after merging this moving the function out of the if would be useful.

Probably a good idea, yes.
I'll have a look at it in the next days...