Skip to content

Started adding spec for coverage - #217

Merged
eregon merged 1 commit into
ruby:masterfrom
donv:try_add_coverage_spec
Mar 26, 2016
Merged

Started adding spec for coverage#217
eregon merged 1 commit into
ruby:masterfrom
donv:try_add_coverage_spec

Conversation

@donv

@donv donv commented Mar 26, 2016

Copy link
Copy Markdown
Contributor

My first contribution to ruby/spec, so I expect to have to change a lot.

First question is that the specs seem organized around methods, but coverage is typically the interaction between the Coverage.start and Coverage.result methods. How should the specs for this interaction be organized?

@eregon

eregon commented Mar 26, 2016

Copy link
Copy Markdown
Member

Hi @donv!
The current organization seems fine, I believe there is no strict rule.
Specifying result in the start spec would seem strange to me.

Comment thread library/coverage/result_spec.rb Outdated
config_file = File.expand_path('../fixtures/start_coverage.rb', __FILE__)

describe "Coverage.result" do
it "needs to be reviewed for spec completeness"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can be removed as soon as there are specs, even if they are not exhaustive.

@eregon

eregon commented Mar 26, 2016

Copy link
Copy Markdown
Member

Looks good overall!
There are a few stylistic issues which I commented.

@donv
donv force-pushed the try_add_coverage_spec branch 3 times, most recently from e6ec9e8 to ea2e8a7 Compare March 26, 2016 18:53
@donv

donv commented Mar 26, 2016

Copy link
Copy Markdown
Contributor Author

OK, applied all the comments except the separate spec for disabled coverage since there is no query.

Looks good?

@donv

donv commented Mar 26, 2016

Copy link
Copy Markdown
Contributor Author

What's the deal with the failing AppVeyor check?

@donv

donv commented Mar 26, 2016

Copy link
Copy Markdown
Contributor Author

Ah! It is for windows. Does this mean MRI on windows is failing Ruby Spec?

Comment thread library/coverage/result_spec.rb Outdated

describe 'Coverage.result' do
before :all do
@config_file = File.expand_path('../fixtures/start_coverage.rb', __FILE__)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is an actual fixture helper which should do exactly the same 😉 , as in https://github.com/ruby/spec/blob/master/core/module/autoload_spec.rb#L18.

@eregon

eregon commented Mar 26, 2016

Copy link
Copy Markdown
Member

Ah! It is for windows. Does this mean MRI on windows is failing Ruby Spec?

Rather than Ruby Spec is still having too many expectations (on Unix features) for Windows.

@donv
donv force-pushed the try_add_coverage_spec branch from 55095a6 to dbaf24f Compare March 26, 2016 19:46
@@ -0,0 +1,3 @@
2 + 2
Coverage.start

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What does the Coverage.start does here? It seems only line 1 is reported?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is to test coverage for the file that calls Coverage.start.

@eregon

eregon commented Mar 26, 2016

Copy link
Copy Markdown
Member

Thank you for your contribution!

@eregon
eregon merged commit 490cc2d into ruby:master Mar 26, 2016
@donv
donv deleted the try_add_coverage_spec branch March 26, 2016 19:58
@donv

donv commented Mar 26, 2016

Copy link
Copy Markdown
Contributor Author

Thank you for this vital project!

@eregon eregon mentioned this pull request Oct 11, 2016
51 tasks
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.

2 participants