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

Sat Jul 6 15:54:35 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).
> 
> Jonathan Marten wrote:
>     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?
>

Perfect, a good test is a test that fails initially.

Yes, please commit the additional test.
Maybe use a username that is more guaranteed not to exist, e.g. "does_not_exist".

- David

-----------------------------------------------------------
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/992efdb4/attachment.htm>