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.

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

Add fixes to dirwalk tests #1061

Merged
merged 2 commits into from May 16, 2017

Conversation

Projects
None yet
4 participants
@chu11
Copy link
Contributor

chu11 commented May 16, 2017

Hit this on catalyst:

FAIL: test_dirwalk.t 20 - breadth-first visited directories with DIRWALK_REALPATH works

Debugged a bit, had to fix what I think is a corner case in the debug output:

@@ -158,11 +158,12 @@ int check_zlist_order (zlist_t *l, const char *base, char *expected[])
             BAIL_OUT ("asprintf");
 
         result = strcmp (exp, dir);
-        free (exp);
         if (result != 0) {
-            diag ("check_zlist: %d: expected %s got %s", i, expected[i], dir);
+            diag ("check_zlist: %d: expected %s got %s", i, exp, dir);
+            free (exp);
             return 0;
         }
+        free (exp);
         i++;
         dir = zlist_next (l);
     }

Result:

# zlist_order: 0: /tmp/achu/dirwalk_test.NdXBNK
# check_zlist: 0: expected /var/tmp/achu/dirwalk_test.NdXBNK got /tmp/achu/dirwalk_test.NdXBNK
not ok 20 - breadth-first visited directories with DIRWALK_REALPATH works
#   Failed test 'breadth-first visited directories with DIRWALK_REALPATH works'
#   at test/dirwalk.c line 297.

My TMPDIR happens to default to /var/tmp, which is a symlink. Changing it to /tmp resolves the error.

So looks like the base directory needs to be "REALPATH"'d too before being passed into the check zlist function.

chu11 added some commits May 15, 2017

libutil/test: Fix dirwalk test corner case
Base directory in test comparison must also be "REALPATH".

@chu11 chu11 requested a review from grondo May 16, 2017

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 16, 2017

Codecov Report

Merging #1061 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1061      +/-   ##
==========================================
+ Coverage   77.81%   77.85%   +0.04%     
==========================================
  Files         148      148              
  Lines       25771    25771              
==========================================
+ Hits        20053    20064      +11     
+ Misses       5718     5707      -11
Impacted Files Coverage Δ
src/cmd/flux-event.c 65.55% <0%> (-1.12%) ⬇️
src/common/libflux/handle.c 85.35% <0%> (-0.57%) ⬇️
src/common/libflux/rpc.c 92% <0%> (ø) ⬆️
src/common/libflux/message.c 81.92% <0%> (+0.11%) ⬆️
src/common/libflux/dispatch.c 83.51% <0%> (+0.27%) ⬆️
src/common/libutil/dirwalk.c 90.71% <0%> (+0.71%) ⬆️
src/broker/content-cache.c 73.85% <0%> (+1.3%) ⬆️
src/broker/module.c 83.56% <0%> (+1.41%) ⬆️
@coveralls

This comment has been minimized.

Copy link

coveralls commented May 16, 2017

Coverage Status

Coverage increased (+0.04%) to 78.102% when pulling 2d3a521 on chu11:dirwalktest into 23facbe on flux-framework:master.

@grondo

grondo approved these changes May 16, 2017

Copy link
Contributor

grondo left a comment

Yes, this looks correct, sorry about that!

I should probably addtests explicitly for corner cases like links, missing directories, etc.

@grondo grondo merged commit 50fe8ab into flux-framework:master May 16, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 23facbe...2d3a521
Details
codecov/project 77.85% (+0.04%) compared to 23facbe
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 78.102%
Details

@grondo grondo referenced this pull request Aug 23, 2017

Closed

0.8.0 Release #1160

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.