Review Request 111390: kshorturifilter: inverted condition in home directory handling

Sat Jul 6 15:51:32 BST 2013

> On July 5, 2013, 2:27 p.m., David Faure wrote:
> > Sounds like the unittest is incomplete then, if it didn't catch the error with ~ or ~user.
> > 
> > 
> > (I don't mean testing the case where the home dir is empty, I only mean the normal case, which you say was going into an error path).

Having looked at the unittest, it doesn't test for that condition.  It would only be triggered in the case of '~user' where the user is valid (and not '~' alone because that is a separate code path).  The unittest only checks for '~foobar', a nonexistent user, so user.isValid() is false and the second condition would never be tried.

The case that does exercise this code would be '~user' where the user does exist;  for example testing for '~bin'.  Running the test with the unmodified code does give a test fail:

QDEBUG : KUriFilterTest::localFiles() qttest(11024)/kurifilter (plugins) KShortUriFilter::filterUri: "~bin"
QDEBUG : KUriFilterTest::localFiles() qttest(11024) filter: *** Result: Encountered error => ' "/bin" '
QDEBUG : KUriFilterTest::localFiles() qttest(11024) filter: Reason: "<html><b>bin</b> does not have a home folder.</html>"
QSYSTEM: KUriFilterTest::localFiles() qttest(11024): ~bin Got URI type ERROR expected LOCAL_DIR 
FAIL!  : KUriFilterTest::localFiles() Compared strings are not the same
   Actual (s_uritypes[filterData->uriType()]): ERROR
   Expected (s_uritypes[expectedUriType]): LOCAL_DIR
   Loc: [/ws/trunk/kdebase/kderuntime/kurifilter-plugins/tests/kurifiltertest.cpp(109)]

but with the modified code the test passes.

Should I commit an additional test of

    filter( "~bin", 0, KUriFilterData::LocalDir, QStringList( "kshorturifilter" ) );

to make the coverage complete?

- Jonathan

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111390/#review35635
-----------------------------------------------------------

On July 4, 2013, 6:32 p.m., Jonathan Marten wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111390/
> -----------------------------------------------------------
> 
> (Updated July 4, 2013, 6:32 p.m.)
> 
> 
> Review request for KDE Runtime.
> 
> 
> Description
> -------
> 
> While investigating URI filtering/validation elsewhere I spotted some odd logic here.  The original line in question seems to be saying:
> 
> if (user is valid && user's home directory is empty)
> {
>   // replace from '~' onwards with home directory
> }
> else
> {
>   // generate an error
> }
> 
> The second condition, though, is surely wrong.  It should say:
> 
> if (user is valid && user's home directory is NOT empty)
> ...
> 
> 
> Diffs
> -----
> 
>   kurifilter-plugins/shorturi/kshorturifilter.cpp d27b018 
> 
> Diff: http://git.reviewboard.kde.org/r/111390/diff/
> 
> 
> Testing
> -------
> 
> Build kderumtime with this change.  Checked correct results for '~' and '~user', where the user both exists and does not, in konqueror and krunner.  Ran the kurifiltertest with all passes.
> 
> 
> Thanks,
> 
> Jonathan Marten
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20130706/04e10426/attachment.htm>