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

Check api functions to increase Code Coverage #6

Merged
merged 8 commits into from
Jun 19, 2021
Merged

Conversation

kpiyush04
Copy link
Contributor

@kpiyush04 kpiyush04 commented May 31, 2021

Increased the code coverage by adding the test cases which includes the test the cli output and works best with snapshot tests, httr library functions to test the to make a request, first load httr, then call GET() with a URL, expect_error() to test the code throw an error, warning, message, or other condition, and expect_equal() function to check the code e return the expected value or not.

#4

Copy link
Owner

@llrs llrs left a comment

Choose a reason for hiding this comment

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

The setup-bugRzilla.R file is special in the sense that it should only contain configuration data or functions needed for tests, not the tests itself. New tests should be on their own testing file, use use_test() (from devtools via usethis pacakge) to create a new test file. All tests that are related should be on the same file, create as much test files as needed.

use_cassette is only needed if you want to mock the response of the website, if there isn't any call to online resource it shouldn't be used.

I provided more inline feedback on the PR. Try to explain the major decision you made on the PR. Why and how does it help to the goal. No need to say I added this test that increases the coverage 5%, but explain I made this test that way because it is easier/better for X reason.

DESCRIPTION Outdated Show resolved Hide resolved
tests/testthat/setup-bugRzilla.R Outdated Show resolved Hide resolved
tests/testthat/setup-bugRzilla.R Outdated Show resolved Hide resolved
tests/testthat/setup-bugRzilla.R Outdated Show resolved Hide resolved
Copy link
Owner

@llrs llrs left a comment

Choose a reason for hiding this comment

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

Still not quite there, but improving on the right direction. I've made some comments on specific points, note that some might be applicable to the whole test-api.R file.

tests/testthat/setup-bugRzilla.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/setup-bugRzilla.R Show resolved Hide resolved
Copy link
Owner

@llrs llrs left a comment

Choose a reason for hiding this comment

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

Great improvement! When you fix the points mentioned I think we can merge this PR. You have detected some some issues detected on the latest changes:

  • ask_confirmation on non interactive environments.
  • params robustness.

I think it would be better if these are separate PR and also a new PR to raise testing coverage of post_help and post_bug.

tests/testthat/test-get_fields.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-bug_search.R Outdated Show resolved Hide resolved
tests/testthat/test-post_help.R Show resolved Hide resolved
@llrs
Copy link
Owner

llrs commented Jun 17, 2021

I think there was something I needed to check from test but now I don't remember. Let me know if there is anything blocking you.

Could you resolve the comments I made?

@kpiyush04
Copy link
Contributor Author

kpiyush04 commented Jun 17, 2021

I think there was something I needed to check from test but now I don't remember. Let me know if there is anything blocking you.

Yes, a few of them I've resolved currently working test-post_help.R file.

@llrs
Copy link
Owner

llrs commented Jun 17, 2021

Let me see the progress. Please update this PR (don't forget to answer/mark as solved the comments 😉 )
You can continue the work on post_help.R on a new PR.

Copy link
Owner

@llrs llrs left a comment

Choose a reason for hiding this comment

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

Some of the issues I mentioned are not fixed and some old issues reappeared.

DESCRIPTION Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
tests/testthat/test-api.R Outdated Show resolved Hide resolved
@llrs llrs changed the title Increased Code Coverage Check api functions to increase Code Coverage Jun 19, 2021
@llrs llrs merged commit 9450c95 into llrs:master Jun 19, 2021
@kpiyush04 kpiyush04 deleted the patch-1 branch June 19, 2021 19:37
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