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 hash commands #38

Merged
merged 9 commits into from Oct 24, 2018

Conversation

Projects
None yet
2 participants
@armstrtw
Contributor

armstrtw commented Oct 23, 2018

add other basic hash functions
closes #37

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 23, 2018

Most excellent that you added tests.

I presume this passes R CMD check at your end? I'll take a closer look this evening....

@armstrtw

This comment has been minimized.

Contributor

armstrtw commented Oct 23, 2018

test suite passes on my end. not sure what's going on. I'll try to dig into travis later.

@armstrtw

This comment has been minimized.

Contributor

armstrtw commented Oct 23, 2018


Test file: /usr/lib64/R/library/RcppRedis/tests/runit.hashTests.R
test_01_setup: (0 checks) ... OK (0 seconds)
test_02_hset: (1 checks) ... OK (0 seconds)
test_03_hset_2: (1 checks) ... OK (0 seconds)
test_04_hget: (1 checks) ... OK (0 seconds)
test_05_setup: (1 checks) ... OK (0 seconds)
test_06_hlen: (1 checks) ... OK (0 seconds)
test_07_hkeys: (1 checks) ... OK (0 seconds)
test_08_hgetall: (1 checks) ... OK (0 seconds)
test_09_hdel: (1 checks) ... OK (0 seconds)
test_09_hdel_1: (1 checks) ... OK (0 seconds)
test_09_hdel_2: (1 checks) ... OK (0 seconds)
test_09_hdel_3: (1 checks) ... OK (0 seconds)

I have a more recent distro at home. I'll give it a shot later.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 23, 2018

Ok, with a bid of prodding (needed to install rredis which seemingly I no longer had) and actually running redis-server (doh!) it runs.

Look closely at tests/runUnitTests.R though -- you may need to set an env var to actually run tests. That var is enforced in .travis.yml so that could be part of it.

I am seeing a little bit of noise in your tests 7 and 8 but they end up with a pass.

@armstrtw

This comment has been minimized.

Contributor

armstrtw commented Oct 23, 2018

yes, I'm setting uncommitted locally to get the suite to run:
-runTests <- FALSE
+runTests <- TRUE

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 23, 2018

I have a super-old helper script I use to run individual scripts. Installed as ~/bin/runitsh I can do ~/bin/runit.sh -p RcppRedis runit.Scriptname.R for various script names. Try that.

#!/bin/sh

set -u
set -e

progname=`basename $0`
options='p:ah?'

usage_and_exit()
{
    echo ""
    echo "Usage: $progname [-p package[,package2,..]] [-?|-h]"
    echo "  Run unit test script for R package"
    echo ""
    echo "Options:"
    echo "  -p package[,package2,..]]  load additional package(s)"
    echo "  -h                         show this help"
    echo "  -a                         run with 'export RunAllRcppTests=yes'"
    echo ""
    echo "The RUnit and inline packages are automatically loaded."
    echo ""
    exit 0
}

while getopts "$options" i
do
    case "$i" in
        p)
            pkg=",$OPTARG"
            shift
            shift
            ;;
        a)
            export RunAllRcppTests="yes"
            shift
            ;;
        h|?)
            usage_and_exit
            ;;
    esac
done

if [ ! -f $1 ]; then
    echo "Error: No file '$1' found"
    exit 1
fi

file=`pwd`/$1

#r -i -t -linline,RUnit${pkg} -e"cppfunction <- function(...) cxxfunction(..., plugin=\"Rcpp\"); runTestFile(\"$file\")"
r -t -lRUnit${pkg} -e"runTestFile(\"$file\")"
@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 24, 2018

This comes back in a different order for me:

R> res <- redis$hkeys(hashname)
R> res
[1] "cat" "hat"
R>  checkEquals(res, c(xname,yname))
Error in checkEquals(res, c(xname, yname)) : 2 string mismatches

so I suggest something like testing this against c(1,2) or something

R> sort(match(res, c(xname,yname)))
[1] 1 2

Or maybe sort() both the returned result, and the comparison set, and test that.

Similarly here:

R> res <- redis$hgetall(hashname)
R> res
$cat
 [1] -0.6907126 -0.6411130  0.7017554  0.3448584  0.4657207 -0.0544486 -0.4507790 -0.4544461  1.6455678  0.6807456

$hat
 [1] -0.1404888 -0.4863224 -1.2442523  0.0332487 -0.6447859 -0.4333763  1.8892861 -0.9328419 -0.3047753 -0.1825650

R> checkEquals(res,  structure(list(xdata,ydata),names=c(xname,yname)))
Error in checkEquals(res, structure(list(xdata, ydata), names = c(xname,  : 
  Names: 2 string mismatches
Component 1: Mean relative difference: 1.64501
Component 2: Mean relative difference: 1.60271
R>
@armstrtw

This comment has been minimized.

Contributor

armstrtw commented Oct 24, 2018

aha. Ok. I'll change the test and re-submit. Likely Thursday, I'll be tied up tomorrow. thx for digging into this.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 24, 2018

Make it 'tests' -- I'd change 7 and 8.

(And in hindsight, we were crazy there. We do not need one function per testing equation. Could all be one or two. But now that it is written...)

I'll also clean up the test runner in tests/ and make the rredis truly optional once you committed.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 24, 2018

GitHub keeps reminding us that

Add more commits by pushing to the add-hash-commands branch on armstrtw/rcppredis.

is possible so I just did that. It's a minimal change -- just glance at it and nod or yell nopes.

@armstrtw

looks good to me. and thanks.

@armstrtw

This comment has been minimized.

Contributor

armstrtw commented Oct 24, 2018

I pulled in your changes locally. passes all tests.

@eddelbuettel eddelbuettel merged commit 31c6ef1 into eddelbuettel:master Oct 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment