search

metricfu / metric_fu

A fist full of code metrics
http://metricfu.github.com/metric_fu
MIT License
624 stars 96 forks source link

Fix test warnings #289

Closed jkeam closed 7 years ago

jkeam commented 7 years ago

Addresses https://github.com/metricfu/metric_fu/issues/288.

The original error thrown was a RuntimeError and it seemed appropriate given the name of the method that a NotImplementedError should be thrown here instead.

Also, as per issue 288, we need to add the correct expectation to match the error thrown to eliminate the warnings you see when running the tests.

jkeam commented 7 years ago

@bf4 and @bergholdt: Thoughts on the appveyor build failing? I'm not familiar with appveyor.

appveyor

bf4 commented 7 years ago

@jkeam It's not configured yet, I believe is the issue.

Here's the appveyor.yml from another project

version: '{build}'

skip_tags: true

environment:
  JRUBY_OPTS: "--dev -J-Xmx1024M --debug"
  matrix:
    - ruby_version: "Ruby21"
    - ruby_version: "Ruby21-x64"
    - ruby_version: "jruby-9.0.0.0"

cache:
  - vendor/bundle

install:
  - SET PATH=C:\%ruby_version%\bin;%PATH%
  - gem install bundler
  - bundle env
  - bundle install --path=vendor/bundle --retry=3 --jobs=3

test_script:
  - bundle exec rake ci

build: off
bergholdt commented 7 years ago

@jkeam I changed the build from msbuild to script - else will look for a visual studio solution file.

But to do some actual work it will need the appveyor.yml file

You can look at #285 branch feature/windows-ci

jkeam commented 7 years ago

Ah, should we go ahead and merge in #285? Is there any more work to be done on it?

jkeam commented 7 years ago

Looking at the test logs from the build in appveyor in #285 everything looks good (https://ci.appveyor.com/project/bf4/metric-fu/build/1.0.7). I'd be ok with merging that in if it's all done.

bergholdt commented 7 years ago

I think #285 could use the matrix from example above - but we could add that later.

But should we we not merge this #289 before or would you like to verify it on appveyeor before?

jkeam commented 7 years ago

I'm ok with merging this in first or after. But I'd love to get rid of those pesky warning messages so the sooner the better :)