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

Upgrade Reek Dependency #306

Merged
merged 18 commits into from
May 1, 2020
Merged

Upgrade Reek Dependency #306

merged 18 commits into from
May 1, 2020

Conversation

etagwerker
Copy link
Member

Hey @dbwest @bf4,

This PR fixes #305 by bumping the dependency of reek and changing the implementation of reek's generator.

I tested that this works in this project: https://github.com/fastruby/e-petitions/tree/metric_fu

It now generates reek's report:

$ bundle exec metric_fu
******* STARTING METRIC churn
******* ENDING METRIC churn
******* STARTING METRIC reek
******* ENDING METRIC reek
******* STARTING METRIC saikuro
******* ENDING METRIC saikuro
******* STARTING METRIC flog
...

Please check it out.

Thanks!

Copy link
Member

@lubc lubc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etagwerker Looks good! 👍 I only added one minor comment

@@ -99,9 +100,12 @@
smell_type: "Duplication",
lines: [6, 9])
]
@examiners = [
instance_double(Reek::Examiner, smells: @smells)
]
@lines = instance_double(@examiner, smells: @smells)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etagwerker is it necessary to keep this variable? I noticed that you removed it in the line below

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. I'll review this today.

@etagwerker etagwerker mentioned this pull request Sep 13, 2019
@etagwerker etagwerker force-pushed the reek-dep branch 2 times, most recently from e42d85b to d0acf59 Compare September 13, 2019 15:58
etagwerker added a commit to fastruby/metric_fu that referenced this pull request Sep 15, 2019
Switching between directories (spec/dummy) and root dir is causing flakiness problems.

If you want to see some of the flakiness, check out the failures in this PR: metricfu#306
@etagwerker
Copy link
Member Author

@bf4 After debugging the Appveyor failure, it seems that I'd have to continue upgrading other dependencies. Not sure if that is a good idea, as this PR will continue to grow in size. I had to merge #308 inside this PR in order to get most of the builds to pass.

What do you suggest?

appveyor.yml Outdated

install:
- set PATH=C:\Ruby%RUBY_VERSION%\bin;%PATH%
- gem install bundler
- bundle install --path=vendor/bundle --retry=3 --jobs=3
- bundle update
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bundle update after bundle install seems odd, given there's no gemfile.lock

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4 I agree, this was a failed attempt at fixing this failure in appveyor: https://ci.appveyor.com/project/etagwerker/metric-fu/build/job/c18jk3upjrk6fw4k

The weird part is that things work as expected when I locally test with ruby-2.6.2:

$ rm Gemfile.lock
$ rvm-prompt
ruby-2.6.2@metric_fu
$ bundle install
...
Bundle complete! 10 Gemfile dependencies, 76 gems now installed.
Gems in the group guard were not installed.
Use `bundle info [gemname]` to see where a bundled gem is installed.
$ bundle exec rspec
NOTE: Gem::Specification#default_executable= is deprecated with no replacement. It will be removed on or after 2018-12-01.
Gem::Specification#default_executable= called from /Users/etagwerker/Projects/fastruby/metric_fu/metric_fu.gemspec:37.
NOTE: Gem::Specification#has_rdoc= is deprecated with no replacement. It will be removed on or after 2018-12-01.
Gem::Specification#has_rdoc= called from /Users/etagwerker/Projects/fastruby/metric_fu/metric_fu.gemspec:40.
"setting timeout 15.0 seconds"
Run options:
  include {:focus=>true}
  exclude {:slow=>true}

All examples were filtered out; ignoring {:focus=>true}

Randomized with seed 25211
........................................................................................./Users/etagwerker/Projects/fastruby/metric_fu/lib/metric_fu/cli/parser.rb:69: warning: constant ::Fixnum is deprecated
......................................................***...*................................................................................................................................................................................./Users/etagwerker/Projects/fastruby/metric_fu/lib/metric_fu/cli/parser.rb:69: warning: constant ::Fixnum is deprecated
....................................................

Pending: (Failures listed here are expected and do not affect your suite's status)

  1) MetricFu::HotspotAnalyzer #hotspots aka worst_items
     # Not yet implemented
     # ./spec/metric_fu/metrics/hotspots/hotspot_analyzer_spec.rb:7

  2) MetricFu::HotspotAnalyzer should have its own tests regarding how it orchestrates the analysis of results, rankings, tables, and analyzed_problems
     # Not yet implemented
     # ./spec/metric_fu/metrics/hotspots/hotspot_analyzer_spec.rb:5

  3) MetricFu::HotspotAnalyzer #analyzed_problems
     # Not yet implemented
     # ./spec/metric_fu/metrics/hotspots/hotspot_analyzer_spec.rb:9

  4) MetricFu::Table needs tests
     # Not yet implemented
     # ./spec/metric_fu/metrics/hotspots/analysis/table_spec.rb:5


Top 10 slowest examples (0.93884 seconds, 50.3% of total time):
  MetricFu::ReekGenerator analyze method with real output, not mocked nor doubled returns real data
    0.52118 seconds ./spec/metric_fu/metrics/reek/generator_spec.rb:177
  The library itself has no malformed whitespace
    0.13179 seconds ./spec/quality_spec.rb:87
  MetricFu::RcovGrapher responding to #get_metrics when metrics have been generated should push to rcov_percent
    0.0758 seconds ./spec/metric_fu/metrics/rcov/grapher_spec.rb:46
  MetricFu::Formatter::HTML given a custom output directory copies common javascripts to the custom output directory
    0.04697 seconds ./spec/metric_fu/formatter/html_spec.rb:110
  SimpleCov::Formatter::MetricFu calculates the same coverage from an RCov report as from SimpleCov
    0.03904 seconds ./spec/metric_fu/metrics/rcov/simplecov_formatter_spec.rb:39
  MetricFu::Formatter::HTML In general creates a data yaml file
    0.02871 seconds ./spec/metric_fu/formatter/html_spec.rb:43
  MetricFu::Formatter::HTML In general creates graphs for appropriate metrics
    0.02614 seconds ./spec/metric_fu/formatter/html_spec.rb:71
  MetricFu given configured metrics, when run creates a report yaml file
    0.02424 seconds ./spec/metric_fu/run_spec.rb:48
  MetricFu::Formatter::HTML given a custom output directory creates templatized html files for each metric in the custom output directory
    0.02347 seconds ./spec/metric_fu/formatter/html_spec.rb:101
  MetricFu::Formatter::HTML In general creates a report index html file
    0.0215 seconds ./spec/metric_fu/formatter/html_spec.rb:50

Top 10 slowest example groups:
  The library itself
    0.07 seconds average (0.13999 seconds / 2 examples) ./spec/quality_spec.rb:10
  MetricFu::ReekGenerator
    0.04445 seconds average (0.53338 seconds / 12 examples) ./spec/metric_fu/metrics/reek/generator_spec.rb:5
  MetricFu::Formatter::HTML
    0.02344 seconds average (0.28125 seconds / 12 examples) ./spec/metric_fu/formatter/html_spec.rb:4
  SimpleCov::Formatter::MetricFu
    0.01364 seconds average (0.04092 seconds / 3 examples) ./spec/metric_fu/metrics/rcov/simplecov_formatter_spec.rb:6
  MetricFu::RcovGrapher
    0.01356 seconds average (0.08138 seconds / 6 examples) ./spec/metric_fu/metrics/rcov/grapher_spec.rb:4
  MetricFu::RcovGenerator configured as rcov
    0.01059 seconds average (0.0953 seconds / 9 examples) ./spec/metric_fu/metrics/rcov/generator_spec.rb:5
  MetricFu
    0.00733 seconds average (0.132 seconds / 18 examples) ./spec/metric_fu/run_spec.rb:4
  Bluff graphers responding to #graph!
    0.00554 seconds average (0.00554 seconds / 1 example) ./spec/metric_fu/reporting/graphs/grapher_spec.rb:3
  MetricFu::AnalyzerTables
    0.00514 seconds average (0.03083 seconds / 6 examples) ./spec/metric_fu/metrics/hotspots/analysis/analyzer_tables_spec.rb:4
  MetricFu::HotspotAnalyzedProblems
    0.00505 seconds average (0.04038 seconds / 8 examples) ./spec/metric_fu/metrics/hotspots/analysis/analyzed_problems_spec.rb:4

Finished in 1.86 seconds (files took 1.83 seconds to load)
379 examples, 0 failures, 4 pending

Randomized with seed 25211

I'm going to get rid of that bundle update

Do you have any suggestions for debugging bundle install in appveyor?

@bf4
Copy link
Member

bf4 commented Sep 16, 2019

Generally changes look fine, but I'd recommend splitting up this PR into the necessary changes and other things you want to do while here to keep it simple

@etagwerker
Copy link
Member Author

@bf4 Good point. I'll try to extract some of the changes into separate pull requests. Here is the first one: #310

@bronzdoc
Copy link

bronzdoc commented Apr 29, 2020

@etagwerker can you rebase this PR? 🙂

…erator

Remove unused variable (`@lines`) in `generator_spec.rb`

Upgrade dependency to `reek`
One of the problems with the RSpec suite is that it is leaking state from one scenario to another. The leakage seems to be related to MetricFu.run_dir

That is why I think it is useful to have this utility method.
Switching between directories (spec/dummy) and root dir is causing flakiness problems.

If you want to see some of the flakiness, check out the failures in this PR: metricfu#306
Usage of spec/dummy is causing flakiness problems. Also, `spec/dummy` seems to be unnecessary.

It is usually a good pattern to use when testing a Rails engine, but this is a Ruby gem and it is probably not a good idea.

I don't understand the difference between spec/dummy and spec/support, so I am trying to get rid of spec/dummy.
This scenario is not the flaky one, but the fact that this is expecting things to work because there was a file under spec/dummy is problematic for other scenarios.
Basic test to make sure that things work as expected
It is causing flakiness as it is leaking state across scenarios.
This should be moved to `.gemspec` in the future

Right now, Appveyor is trying to install `guard` which is not necessary in that environment
@etagwerker
Copy link
Member Author

@bronzdoc Sure, done 👍

This was referenced Apr 29, 2020
@bronzdoc bronzdoc merged commit 125473c into metricfu:master May 1, 2020
@bronzdoc bronzdoc mentioned this pull request May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reek: uninitialized constant Parser::Ruby21 (NameError)
4 participants