greghaskins / spectrum

A BDD-style test runner for Java 8. Inspired by Jasmine, RSpec, and Cucumber.
MIT License
145 stars 23 forks source link

Modified `BlockConfigurationChain` to use typed objects #101

Closed ashleyfrieze closed 7 years ago

ashleyfrieze commented 7 years ago

to represent the configuration, so that it can be extended without (much) code change.

A proposed fix for #100

codecov[bot] commented 7 years ago

Codecov Report

Merging #101 into master will increase coverage by 0.02%. The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #101      +/-   ##
============================================
+ Coverage     99.34%   99.36%   +0.02%     
- Complexity      281      291      +10     
============================================
  Files            31       34       +3     
  Lines           612      632      +20     
  Branches         17       16       -1     
============================================
+ Hits            608      628      +20     
  Partials          4        4
Impacted Files Coverage Δ Complexity Δ
...ghaskins/spectrum/internal/BlockConfiguration.java 100% <100%> (ø) 8 <6> (-4) :x:
...c/main/java/com/greghaskins/spectrum/Spectrum.java 100% <100%> (ø) 45 <4> (ø) :x:
.../greghaskins/spectrum/BlockConfigurationChain.java 100% <100%> (ø) 5 <3> (-1) :x:
...om/greghaskins/spectrum/internal/BlockTagging.java 100% <100%> (ø) 6 <6> (?)
...om/greghaskins/spectrum/internal/BlockFocused.java 100% <100%> (ø) 4 <4> (?)
...com/greghaskins/spectrum/internal/BlockIgnore.java 100% <100%> (ø) 5 <5> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2e4432f...e9328fd. Read the comment docs.

greghaskins commented 7 years ago

@ashleyfrieze nice work! I like this approach much better than what we had. with(tags("foo").and(focus()), () -> {}) reads much better, while also being open-closed. I'm basically ready to merge this PR, unless you have any further tweaks to make.

A couple observations:

  1. It strikes me that we didn't have very strong coverage if this change doesn't require modifying any of the existing tests! 😯 At least we've got one in there now to test the chaining with and().
  2. One of my last remaining tasks for 1.1.0 is to make sure we don't expose any internal interfaces on the Public API (that way we can continue refactoring there with impunity after release). I've got it on my list to finish going through to make sure nothing internal is used as an input or output, and promote any necessary items to the main package. For this PR, I think that would only affect BlockConfigurable.
ashleyfrieze commented 7 years ago

The actual chaining of multiple types of config had dropped out of tests. The reason the tests didn't change is that the facade via the Spectrum class wrapped it all up. In other words, the outward interface has barely changed and the testable behaviour is the same. That's good, I think.

Merge if you like it!