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

Re factor compiler specs to use 'let','match_array','expect',be_nil' #968

Merged
merged 1 commit into from Aug 28, 2013

Conversation

Projects
None yet
4 participants
@rajcybage
Copy link
Contributor

rajcybage commented Aug 23, 2013

fix the should to expect use let use match_array instead of == use be_nil instead of nil also use expect block instead of lamda block

@BanzaiMan

This comment has been minimized.

Copy link
Member

BanzaiMan commented Aug 23, 2013

Please change the subject to use imperative; e.g., "Refactor compiler specs to use 'let'"

@rajcybage

This comment has been minimized.

Copy link
Contributor Author

rajcybage commented Aug 24, 2013

hi @BanzaiMan not only let but also many other like be_nil inspite of nil and also match_array,expect so in commit message should I mention all these thanks

@rajcybage

This comment has been minimized.

Copy link
Contributor Author

rajcybage commented Aug 26, 2013

Done

Thanks you @BanzaiMan

@ghost ghost assigned BanzaiMan Aug 26, 2013

@rajcybage

This comment has been minimized.

Copy link
Contributor Author

rajcybage commented Aug 27, 2013

@rajcybage

This comment has been minimized.

Copy link
Contributor Author

rajcybage commented Aug 28, 2013

If it is acceptable then I can proceed to modify many other rspec codes which are still written in older style

Thanks

BanzaiMan added a commit that referenced this pull request Aug 28, 2013

Merge pull request #968 from rajcybage/refactor_rspec
Re factor compiler specs to use 'let','match_array','expect',be_nil'

@jrubyci jrubyci merged commit 7dbb03e into jruby:master Aug 28, 2013

1 check passed

default The Travis CI build passed
Details

@rajcybage rajcybage deleted the rajcybage:refactor_rspec branch Aug 28, 2013

@Sinjo

This comment has been minimized.

Copy link
Contributor

Sinjo commented Aug 28, 2013

Doesn't changing from the == matcher to match_array makes the tests weaker (they no longer assert on array order)? Wouldn't the eq matcher (which is the same as == but works with the new expect syntax) be the better choice?

My other concern is more to do with test readability: moving the array called "expected" all the way up to the top of the file makes it less clear what that assertion is doing.

Edit: Same goes for "base" being extracted to a let. It's no longer local to where it's being used, and it's not shared between examples so there doesn't seem to be any advantage to using a let.

@rajcybage

This comment has been minimized.

Copy link
Contributor Author

rajcybage commented Aug 29, 2013

Please look into Expected vs should for better rspec http://betterspecs.org/#expect yes all let for :base are used in local codes rajcybage@7dbb03e#L0R141 to rajcybage@7dbb03e#L0R154

@rajcybage

This comment has been minimized.

Copy link
Contributor Author

rajcybage commented Aug 29, 2013

Please look into the time taken by expect less than should

 Benchmark.bm do |x|
   it "adds an edge to newly created graph" do
    x.report("should"){@graph.edges.size.should be 0}

    x.report("expect"){expect(@graph.edges.size).to eq 0}
  end
 end

  $ rspec spec/ir/directed_graph/directed_graph_spec.rb
                 user     system      total        real
      should  0.004000   0.000000   0.004000 (  0.005000)
      expect  0.003000   0.000000   0.003000 (  0.003000)`
@Sinjo

This comment has been minimized.

Copy link
Contributor

Sinjo commented Aug 29, 2013

I'm not disagreeing with the switch from "should" to "expect" (though I wouldn't use that benchmark to justify it, but that's another story*). What I'm saying should be changed is:

  1. Go back from "match_array" to "eq". match_array is the wrong assertion for these tests. The order of the array should be checked.
  2. Get rid of those "let" blocks. They aren't being used to remove repetition of a variable, and moving the example data to the top of the file just makes it less clear what the test is asserting in both cases.

* For those interested in why I don't think those benchmark numbers are a good justification:

  • Even if the numbers are correct, the difference in speed is minimal and unlikely to have a noticeable impact on spec performance.
  • When the time taken the function being benchmarked is so small, you need to run the function many times and take an average of the time taken. There are too many things which could account for a 1ms difference in a function executing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.