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

Improve coverage for googlecharts.rb file #80

Merged
merged 5 commits into from
Mar 1, 2018

Conversation

Prakriti-nith
Copy link
Contributor

@Prakriti-nith Prakriti-nith commented Feb 25, 2018

Partial fix for the issue #67.
Tests were already present for an empty array being passed to the data parameter.
Adding tests when a vector is passed as data.
It also improves coverage for the method add_vector_data: https:/SciRuby/daru-view/blob/master/lib/daru/view/adapters/googlecharts.rb#L154-L160.

I also wanted to add tests for the method return_js_type: https:/SciRuby/daru-view/blob/master/lib/daru/view/adapters/googlecharts.rb#L173-#L188.
I have tried testing it as:
it "when data is of class type String" do expect(Daru::View::Adapter::GooglechartsAdapter.return_js_type("daru")).to eq "string" end
But as this method is a private method and it is recommended not to test the private methods, is it possible if we could test it using ruby's send method?

it "Table class must be GoogleVisualr::DataTable when data objects are" \
" of class Daru::Vector" do
expect(Daru::View::Table.new(@data_vec1, @options).table).to be_a GoogleVisualr::DataTable
end
Copy link
Member

Choose a reason for hiding this comment

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

Please also verify the data and options attribute in Daru::View::Table class.

@Shekharrajak
Copy link
Member

But as this method is a private method and it is recommended not to test the private methods, is it possible if we could test it using ruby's send method?

We cantest it by the method where this private method(return_js_type) is called and execute the private method.

@Shekharrajak
Copy link
Member

Shekharrajak commented Feb 27, 2018

We can check the new coverage report of the file after running bundle exec rspec . Please refer

@Prakriti-nith
Copy link
Contributor Author

Sure. I will do this in a separate PR. Should I make any another changes in this code?

Copy link
Member

@Shekharrajak Shekharrajak left a comment

Choose a reason for hiding this comment

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

Can you add other cases means list of rows, data frame in this PR only ? Please refer comments present in the code or iruby notebook examples for Google charts datatable, to see how to generate table.

@@ -36,5 +58,26 @@
expect(Daru::View::Table.new(@data_vec1, @options).options).to eq @options
expect(Daru::View::Table.new(@data_vec1, @options).data).to eq @data_vec1
end
it "Table class must be GoogleVisualr::DataTable when data objects are" \
" of class Daru::DataFrame" do
expect(Daru::View::Table.new(@data_df, @options).table).to be_a GoogleVisualr::DataTable
Copy link
Member

Choose a reason for hiding this comment

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

Please check line length. You can do something like this :

        expect(Daru::View::Table.new(
          data_frame,
          option)....
        ).to eq ...

end
it "Table class must be GoogleVisualr::DataTable when data objects are" \
" of class Array" do
expect(Daru::View::Table.new(
Copy link
Member

Choose a reason for hiding this comment

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

Isn't better to put Daru::View::Table.new(@data_array2,@ options) into one variable and use it in next 2 line rather than executing it again and again? Follow it in all cases.

@Shekharrajak
Copy link
Member

It is better to use let in this case because of Lazy loaded variables benefit. Please read about let vs instance variable. Merging this for now.

@Shekharrajak Shekharrajak merged commit 9990f7d into SciRuby:master Mar 1, 2018
@Prakriti-nith Prakriti-nith deleted the coverage2 branch May 29, 2018 12:14
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