[SCons] implement 'googletest' option #535

band-a-prend · 2018-05-12T07:41:10Z

Fixes #515

Changes proposed in this pull request:

New scons build option introduced to replace 'system_googletest'
and allow to not use 'googletest' module while running test
(all tests that require this module could be omited)
New option description:

'googletest',
"""Select whether to use gtest/gmock from system
    installation ('system'), from a Git submodule ('submodule'), to decide
    automatically ('default') or don't look for gtest/gmock ('none')
    and don't run tests that depend on gtest/gmock.
    Selection of 'none' will suppress the depricated 'system_googletest' option."""

Old option is still presented and mentioned as 'depricated'.
Option 'googletest = none' supress the old 'system_googletest'.
In other cases if they is setted explicitly and contradict to eacher other - an ERROR message appears.

P.S.
See attached file 'scons_test_results.txt' to compare results of running 'scons test' command before and after implimentation of new option.

speth

I think it would help to structure this such that after the initial checks, we only look at the googletest variable, which should have as its value one of system, submodule, or none, and all later references to the system_googletest variable should be removed.

When it comes to running the test suite, I think there need to be some very loud and clear warnings that these tests are being skipped, printed in the test summary at the very end.

speth · 2018-05-12T19:55:59Z

SConstruct

@@ -538,8 +538,16 @@ config_options = [
        'system_googletest',
        """Select whether to use gtest/gmock from system
           installation ('y'), from a Git submodule ('n'), or to decide
-           automatically ('default').""",
+           automatically ('default'). Depricated option, please use 'googletest' instead.""",


The correct spelling is 'Deprecated'.

speth · 2018-05-12T20:05:04Z

SConstruct

+            config_error('Expected system installation of Googletest-1.8.0, but it '
+                         'could not be found.')
+    elif env['system_googletest'] == 'n' and env['googletest'] == 'system':
+        config_error('Options "googletest=system" and "system_googletest=n" contradict each other.\n'


I would be fine with having the googletest option always override the system_googletest option, with no need to check for consistency.

speth · 2018-05-12T20:06:38Z

SConstruct

@@ -538,8 +538,16 @@ config_options = [
        'system_googletest',
        """Select whether to use gtest/gmock from system
           installation ('y'), from a Git submodule ('n'), or to decide
-           automatically ('default').""",
+           automatically ('default'). Depricated option, please use 'googletest' instead.""",
        'default', ('default', 'y', 'n')),


It might be useful to introduce a new default here, e.g. unspecified, treated as equivalent to default, and print a warning if any of the current options are explicitly given, default included.

I still don't implement this. Could you clarify what is the warning message should be?
That "it's will be skipped if 'googletest' is set explicitly" and if not then notify that "this option is deprecated and in future user should use another"?

And about your offering about additional message

When it comes to running the test suite, I think there need to be some very loud and clear warnings that these tests are being skipped, printed in the test summary at the very end.

It seems that I need to edit 'buildutils.py' and I currently have no idea how to implement this behaviour.
So far I added additional warning before running tests after the ending of Cantera build process.

Sorry for delaying with my answer. That's usually a problem for me to edit and rebase commits on weekdays.

speth · 2018-05-12T20:08:55Z

SConstruct

+        has_gtest = conf.CheckCXXHeader('gtest/gtest.h', '""')
+        has_gmock = conf.CheckCXXHeader('gmock/gmock.h', '""')
+        if has_gtest and has_gmock:
+            env['system_googletest'] = True


Should be setting env['googletest'] = 'system' here, right?

Yes, you are right. Also I add string env['googletest'] = 'submodule' to another block below.

speth · 2018-05-12T20:10:57Z

test/SConscript

@@ -15,11 +15,15 @@ else:

 localenv.Prepend(CPPPATH=['#include'],
                 LIBPATH='#build/lib')
-if not env['system_googletest']:
-    localenv.Prepend(CPPPATH=['#ext/googletest/googletest/include',
+if env['googletest'] in ('system', 'submodule', 'default'):


Once we know whether there is either a system or submodule googletest, we should no longer be looking for default

Removed 'default' here.

speth · 2018-05-12T20:17:37Z

test/SConscript

-    env.Depends(run_program, env['build_targets'])
-    env.Depends(env['test_results'], run_program)
-    Alias('test-%s' % progName, run_program)
+    if env['googletest'] in ('system', 'submodule', 'default'):


More cleanly written as if env['googletest'] != 'none':

Done. Yes it's more clarifying condition.

band-a-prend · 2018-05-19T00:50:36Z

I made some updates in SCounstruct and test/SConscript.

About item 3 in your "changes request":

It might be useful to introduce a new default here, e.g. unspecified, treated as equivalent to default, and print a warning if any of the current options are explicitly given, default included.

I still don't implement this. Could you clarify what is the warning message should be?
That "it's will be skipped if 'googletest' is set explicitly" and if not then notify that "this option is deprecated and in future user should use another"?

And about your offering about additional message:

When it comes to running the test suite, I think there need to be some very loud and clear warnings that these tests are being skipped, printed in the test summary at the very end.

It seems that I need to edit 'buildutils.py' and I currently have no idea how to implement this behaviour.
So far I added additional warning before running tests after the ending of Cantera build process.

Sorry for delaying with my answer. That's usually a problem for me to edit and rebase commits on weekdays.

band-a-prend · 2018-05-19T01:12:28Z

@speth

Could you check why OSX "The Travis CI build" is failed?
The Linux build was completed successfully.

band-a-prend · 2018-05-19T09:56:04Z

I'm in a shame spiral. The build problem was due to I inserted few tabulations in two strings while editing.

speth

Thanks, this is looking quite a bit cleaner now. I think there's still a bit that can be done to help with the transition from the system_googletest to googletest option. If we introduce unspecified as the default choice for the system_googletest option, taking this to mean that the user has not given any of the three existing choices, then I think logic should be something like the following:

if env['system_googletest'] != 'unspecified':
    warn about deprecated option
if env['system_googletest'] == 'default':
    env['googletest'] = 'default'
elif env['system_googletset'] == 'y':
    env['googletest'] = 'system'
elif env['system_googletest'] == 'n':
    env['googletest'] = 'submodule'

After which point the 'system_googletest' variable should not be used at all.

I'll see about putting together a patch that gives the behavior I'd like for running the test suite when use of googletest is disabled.

speth · 2018-05-19T15:07:42Z

test/SConscript

+                    CCFLAGS=env['warning_flags'])
+else:
+    localenv.Append(LIBS=cantera_libs,
+                    CCFLAGS=env['warning_flags'])


Since we append env['warning_flags'] to CCFLAGS regardless, this should be done outside the if/else block.

New scons build option introduced to replace 'system_googletest' and allow to not use 'googletest' module while running test (all tests that require this module could be omited) New option description: 'googletest', """Select whether to use gtest/gmock from system installation ('system'), from a Git submodule ('submodule'), to decide automatically ('default') or don't look for gtest/gmock ('none') and don't run tests that depend on gtest/gmock. If this option is set then it suppress the deprecated 'system_googletest' option.""" Old option is still presented and mentioned as 'deprecated'. Option 'googletest' supresses the old 'system_googletest' one.

speth requested changes May 12, 2018

View reviewed changes

band-a-prend force-pushed the googletest branch from eec05b5 to a7e48e4 Compare May 19, 2018 00:42

band-a-prend force-pushed the googletest branch from a7e48e4 to 3680288 Compare May 19, 2018 09:26

speth reviewed May 20, 2018

View reviewed changes

speth mentioned this pull request May 29, 2018

Fixes for fmtlib 5.0.0 #540

Merged

band-a-prend and others added 2 commits July 6, 2018 17:20

Mark googletest-based tests as failures if googletest is not available

5c09fc5

speth force-pushed the googletest branch from 0553b0c to d983c0a Compare July 6, 2018 21:21

speth approved these changes Jul 6, 2018

View reviewed changes

speth force-pushed the googletest branch 3 times, most recently from 5fdd673 to dcd82e9 Compare July 9, 2018 14:41

[SCons] Improve transition from system_googletest to googletest option

d62c94d

speth force-pushed the googletest branch from dcd82e9 to d62c94d Compare July 9, 2018 15:07

speth merged commit 333d388 into Cantera:master Jul 9, 2018

band-a-prend deleted the googletest branch August 4, 2019 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SCons] implement 'googletest' option #535

[SCons] implement 'googletest' option #535

band-a-prend commented May 12, 2018 •

edited

speth left a comment

speth May 12, 2018

band-a-prend May 19, 2018

speth May 12, 2018

band-a-prend May 19, 2018

speth May 12, 2018

band-a-prend May 19, 2018 •

edited

speth May 12, 2018

band-a-prend May 19, 2018

speth May 12, 2018

band-a-prend May 19, 2018

speth May 12, 2018

band-a-prend May 19, 2018

band-a-prend commented May 19, 2018

band-a-prend commented May 19, 2018

band-a-prend commented May 19, 2018

speth left a comment

speth May 19, 2018

[SCons] implement 'googletest' option #535

[SCons] implement 'googletest' option #535

Conversation

band-a-prend commented May 12, 2018 • edited

Changes proposed in this pull request:

speth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

band-a-prend May 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

band-a-prend commented May 19, 2018

band-a-prend commented May 19, 2018

band-a-prend commented May 19, 2018

speth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

band-a-prend commented May 12, 2018 •

edited

band-a-prend May 19, 2018 •

edited