[#15] Fixes unit test issues #77

tst-ccamp · 2017-04-19T00:39:38Z

Fixes #15

This is a follow up to this PR: #70

googlebot · 2017-04-19T00:39:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

bradh · 2017-04-19T01:36:03Z

Confirm submission IAW CLA.

andreisel · 2017-04-19T20:34:13Z

earth_enterprise/src/common/tests/SConscript

+# Drop in the test data
+fusiontestdir = os.path.join(testdir, 'fusion/testdata')
+shutil.rmtree(fusiontestdir, ignore_errors=True)
+testdatasrc = Dir('../../../fusion/testdata').abspath


Maybe use src-base dir:
testdatasrc = Dir('#fusion/testdata').abspath

andreisel · 2017-04-19T20:41:44Z

earth_enterprise/src/common/tests/SConscript

+fusiontestdir = os.path.join(testdir, 'fusion/testdata')
+shutil.rmtree(fusiontestdir, ignore_errors=True)
+testdatasrc = Dir('../../../fusion/testdata').abspath
+shutil.copytree(testdatasrc, fusiontestdir)


Can use env.rsync_cmd % (testdatasrc, fusiontestdir). It is being used in other SConscript, but shutil is LGTM.

Done. I actually prefer this method because it makes sure that it's executed at the correct build stage, unlike shutil which is executed immediately.

tst-ccamp · 2017-04-19T23:30:55Z

@andreisel I've implemented your suggested changes

tst-lsavoie · 2017-04-20T17:19:01Z

earth_enterprise/src/common/tests/SConscript

@@ -16,6 +16,7 @@

 Import('env')
 import os
+import shutil


This import is no longer needed.

tst-lsavoie

Remove the unused import.

tst-lsavoie

Tested on Ubuntu 14.04 and RHEL 7. All tests passed.
Looks good.

tst-ppenev · 2017-04-20T21:27:35Z

It worked for me, if I ran ./RunAllTests.pl from <src/NATIVE-OPT-x86_64/bin/tests>, but not if I ran it from a different working directory.

I thought the point of using FindBin was to make it independent of the working directory. @tst-ccamp , could we update RunAllTests.pl to change to the directory the script is located in, before running tests?

Optionally, it might be nice to get a warning if you try running <src/common/tests/RunAllTests.pl>, rather than from a build directory.

andreisel · 2017-04-21T02:48:13Z

earth_enterprise/src/common/tests/SConscript

-env.copyfile(os.path.join(env.exportdirs['bin'],'tests'),
-             'RunAllTests.pl')
+# Drop in the test data
+env.Command(


Chris,
maybe put copying of script into the command as well.
So, we will have the command that manages everything we need to run tests.

could be something like this:

# put this somewhere on the top of file. I am expecting it should be a path in bin../..
current_dir = Dir('.').abspath
...
testdir = os.path.join(env.exportdirs['bin'],'tests')

# Copy the test data and the script that run all tests.
test_target = '%s/.install_for_test' % current_dir
env.Command(
test_target, testdir,
[env.copyfile(testdir, 'RunAllTests.pl'),
env.rsync_cmd % (Dir('#fusion/testdata').abspath, '%s/tests/fusion/' % Dir(env.exportdirs['bin']).abspath),
Touch('$TARGET')])

@andreisel I don't really understand why this is necessary. The command to copy the RunAllTests.pl file works fine the way it is. And I think moving it underneath the env.Command call just makes it a little more confusing in my opinion.

This may be because I don't really like the syntax for the env.Command in the first place. The problem is made somewhat worse because scons doesn't have thorough documentation for it. Also, I don't know if env.copyfile will return a "Command" that the env.Command function can use. I think it just does the copy instead, but I could be wrong. Scons is a bit complex still in my mind. :)

Yeah, scons syntax is confusing.
My suggestion is combining all test setup actions into one command. So if anything else is required for test, it will be added to the list in the env.Command().

Why do we need to use/return anything from env.copyfile() in env.Command().

Source in env.Command() seems allow to set some dependencies:
Suggested above:

specifies to run set of commands in the list [..] for target "install_for_test";

specifies dependency on testdir: $SOURCE=testdir;

creates .install_for_test file (TARGET) showing that the target "install_for_test" has been successful.

@andreisel I tried implementing your suggestion. It turns out that the Command builder expects a list of strings, so it tries to execute the return value from env.copyfile as a string. This ends up running the tests as a part of the build, which is not the intended behavior. I think leaving it as two separate steps is the most scons-ish way of doing this.

SG. Thanks.

tst-lsavoie · 2017-04-24T21:12:48Z

@tst-ppenev I added a check to detect when the script is run outside of the expected directory and print an error. Take another look.

andreisel · 2017-04-24T22:57:09Z

earth_enterprise/src/common/tests/SConscript

-env.copyfile(os.path.join(env.exportdirs['bin'],'tests'),
-             'RunAllTests.pl')
+# Drop in the test data
+env.Command(


SG. Thanks.

tst-ppenev · 2017-04-24T23:09:38Z

@tst-lsavoie: I'm rebuilding. I was thinking the script could cd into $scriptDir before running the tests, if necessary, but just a warning could be OK. At least, a new user wouldn't be confused about why the tests fail.

tst-ppenev

@tst-lsavoie: I'm OK with your change. I added a couple of minor proposed improvements. Let me know if you accept them, or not. Otherwise, I'm ready to approve.

tst-ppenev · 2017-04-24T23:52:58Z

earth_enterprise/src/common/tests/RunAllTests.pl

+# This script should be run from NATIVE-*-x86_64/bin/tests
+my $workingDir = abs_path(cwd());
+my $scriptDir = dirname(abs_path($0));
+my $notInBuildDir = (index($scriptDir, "NATIVE") == -1);


Minor: Since Perl is famous for regular expressions, or regular expressions are famous because of Perl, you could use something like:

my $notInBuildDir = $scriptDir !~ /NATIVE-[a-zA-Z0-9_-]*\/bin\/tests$/;

This should work in more cases than (index($scriptDir, "NATIVE") == -1).

tst-ppenev · 2017-04-24T23:59:14Z

earth_enterprise/src/common/tests/RunAllTests.pl

+my $workingDir = abs_path(cwd());
+my $scriptDir = dirname(abs_path($0));
+my $notInBuildDir = (index($scriptDir, "NATIVE") == -1);
+if ($workingDir ne $scriptDir || $notInBuildDir) {


Minor: Instead of testing $workingDir ne $scriptDir you could also do:

chdir($scriptDir);

That way the script would work from anywhere, as long as it's called from a build directory.

In total it would be something like:

# This script should be run from NATIVE-*-x86_64/bin/tests my $scriptDir = dirname(abs_path($0)); chdir($scriptDir); if ($scriptDir !~ /NATIVE-[a-zA-Z0-9_-]*\/bin\/tests$/) { # . . . }

I agree with this. It would be more user friendly in my opinion to just cd into the proper directory.

tst-lsavoie · 2017-04-25T14:46:43Z

@tst-ppenev I tried it with the chdir command and I still got test failures. Rather than try to debug it, I'm just leaving it as a warning. I did add the regex, though.

tst-ppenev · 2017-04-25T19:05:18Z

@tst-lsavoie: You also need to replace the use of the $Bin variable with "." after the chdir. It seems to work for me.

tst-lsavoie · 2017-04-26T16:18:06Z

@tst-ppenev I'm still getting failures. If I run from src it works, but if I run from earth_enterprise (one directory above src) it fails.

tst-lsavoie · 2017-04-26T17:04:22Z

@tst-ppenev figured it out and made the suggested changes. Take another look.

Uses Command/rsync_cmd to match other SConscripts

Running the unit tests from outside the NATIVE-REL-x86_64 (or similar) directory causes failures. This change prints an error if the tests are run outside the expected directory. Adds a regex check to ensure that tests are running from the correct directory. This is more robust than the previous substring check. Changes to the directory containing the test script before running the tests.

Ensure unit tests can find test data (google#15).

94f3e59

tst-ccamp self-assigned this Apr 19, 2017

tst-ccamp requested review from andreisel, tst-lsavoie and tst-ppenev April 19, 2017 00:42

andreisel suggested changes Apr 19, 2017

View reviewed changes

tst-lsavoie reviewed Apr 20, 2017

View reviewed changes

tst-lsavoie suggested changes Apr 20, 2017

View reviewed changes

tst-ccamp force-pushed the bug15 branch from ead8031 to 9c332e1 Compare April 20, 2017 18:12

tst-lsavoie approved these changes Apr 20, 2017

View reviewed changes

andreisel approved these changes Apr 21, 2017

View reviewed changes

andreisel approved these changes Apr 24, 2017

View reviewed changes

tst-ppenev reviewed Apr 25, 2017

View reviewed changes

This was referenced Apr 26, 2017

SConstruct file found #65

Closed

Several fusion gtest unit tests fail on RHEL 7.3 #15

Closed

tst-ccamp and others added 2 commits April 26, 2017 13:23

Updated SConscript to only copy testdata directory (google#15)

e0a7dc7

Uses Command/rsync_cmd to match other SConscripts

tst-ccamp force-pushed the bug15 branch from eb84471 to eb59759 Compare April 26, 2017 18:23

tst-ccamp merged commit 54d9e28 into google:master Apr 26, 2017

tst-ccamp deleted the bug15 branch April 26, 2017 18:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#15] Fixes unit test issues #77

[#15] Fixes unit test issues #77

tst-ccamp commented Apr 19, 2017

googlebot commented Apr 19, 2017

bradh commented Apr 19, 2017

andreisel Apr 19, 2017

tst-ccamp Apr 19, 2017

andreisel Apr 19, 2017

tst-ccamp Apr 19, 2017

tst-ccamp commented Apr 19, 2017

tst-lsavoie Apr 20, 2017

tst-ccamp Apr 20, 2017

tst-lsavoie left a comment

tst-lsavoie left a comment

tst-ppenev commented Apr 20, 2017

andreisel Apr 21, 2017

tst-ccamp Apr 21, 2017

tst-ccamp Apr 21, 2017

andreisel Apr 21, 2017

tst-lsavoie Apr 24, 2017

andreisel Apr 24, 2017

tst-lsavoie commented Apr 24, 2017

andreisel Apr 24, 2017

tst-ppenev commented Apr 24, 2017

tst-ppenev left a comment

tst-ppenev Apr 24, 2017

tst-ppenev Apr 24, 2017

tst-ccamp Apr 25, 2017

tst-lsavoie commented Apr 25, 2017

tst-ppenev commented Apr 25, 2017

tst-lsavoie commented Apr 26, 2017

tst-lsavoie commented Apr 26, 2017

[#15] Fixes unit test issues #77

[#15] Fixes unit test issues #77

Conversation

tst-ccamp commented Apr 19, 2017

googlebot commented Apr 19, 2017

bradh commented Apr 19, 2017

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

tst-ccamp commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tst-lsavoie left a comment

Choose a reason for hiding this comment

tst-lsavoie left a comment

Choose a reason for hiding this comment

tst-ppenev commented Apr 20, 2017

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

tst-lsavoie commented Apr 24, 2017

Choose a reason for hiding this comment

tst-ppenev commented Apr 24, 2017

tst-ppenev 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

tst-lsavoie commented Apr 25, 2017

tst-ppenev commented Apr 25, 2017

tst-lsavoie commented Apr 26, 2017

tst-lsavoie commented Apr 26, 2017