unit testing parser and git modules

[malialiu]

Unit tests for git_checkout and parse_gitlog

[carlosparadis]

Please remember to include these pull requests on the board (right menu), under review, when you update here, or I may run the risk of not noticing there are more to be reviewed. @leilani-reich can help you with this if you are not clear how. I just added it to the backlog for you.

[carlosparadis]

Hi @malialiu

I added a few more comments, but you are almost there on the unit tests. When you make the next commit, please git squash them all into one commit.

Also, you want to separate your tests into the appropriate (new) files:

git_checkout tests should exist in a test-git.R file, whereas the parser_gitlog tests should exist in a test-parser.R file. The file names should match the file names of where the function belongs.

[carlosparadis]

Hi,

You should be careful about merging commits after I make code revisions and before you implement them. Note my review now lies in an "outdated" category. If it is clear to you what is the change request, that is fine.

[carlosparadis]

Maybe all that is missing is the test_that("test that"). Just remove the extra "test that" from the string. :)

[leilani-reich]

Hi @carlosparadis, I have not assigned Nico to Malia's unit tests because she says she thinks all the revisions are done. Or should I anyway? Thanks.

[carlosparadis]

I am not sure I understand what "all revisions are done" means. Did Malia already do the 3 Asks we discussed? I am planning to read them later today to confirm, but if not, then she still has work to do on this Pull Request, no?

[leilani-reich]

This is the unit testing for milestone 1. You asked me to assign the milestone 1 pending pull requests to Nico, right?

Edit: No Malia has not done the asks yet.

[carlosparadis]

Are all yours, Malia's, and Nico's unit tests included in this Pull Request that is listed in https://github.com/sailuh/kaiaulu/issues/154#issue-1597495593 (and or any other unit tests of milestone 1)?

[carlosparadis]

To be clear, Nico's task for Saturday is to finish Milestone 1 unit tests for the entire group. He should look through all the pull request comments (with hopefully your guidance so he knows what was already fixed and what is not), and should then make any remaining changes I asked so I can make a final review pass on them and hopefully integrate.

For example, I believe Malia's unit test still says test_that("test that.....") etc. The unit tests should be normalized so they are consistent to Kaiaulu codebase. Hence, one person make revisions to all unit tests to ensure consistency so we don't end up in the first iteration of DV8 situation where Nico used rstudioapi, while you and Malia used system2 for example.

[leilani-reich]

Yes, those are the only unit tests for milestone 1. As for milestone 2, the bugzilla crawler unit test task is mentioned here https://github.com/sailuh/kaiaulu/issues/153#issue-1587260581 and the bugzilla crawler unit test task here https://github.com/sailuh/kaiaulu/issues/152.

Also, I believe Malia's most recent commit to this pull request addressed the "test that".

[carlosparadis]

Maybe I am getting confused about "Malia's revisions are done". I originally thought you were referring to the Bugzilla Notebook, but I guess you were just referring to Malia's Milestone 1 unit tests. Is that correct?

What of yours and Nico's unit tests? Are all revisions done too? Meaning, this single pull request contains all unit tests of Milestone 1 with all revisions done and ready to merge? If this is the case, then Nico's next week's deadline becomes this weeks deadline: He should implement milestone 2 unit tests by Saturday, the 22nd. I can then decide on the subsequent week task at our meeting. The process and the deadlines remain the same, only the tasks will change.

Let me know whether Nico still has revisions to do for Milestone 1, or otherwise, if he is finishing Milestone 2 unit tests by the 22nd. Either way, he should be assigned a deliverable for the 22nd if nothing is left of milestone 1.

[leilani-reich]

Yes, I'm just referring to the milestone 1 unit tests. I think Nico's revisions for milestone 1 were mostly done as well, but I know my unit tests are not. I never got to making any revisions for my milestone 1 tests here https://github.com/sailuh/kaiaulu/pull/162.

Also, to be clear on associated milestone 1 pull requests:

• originally mine (I reassigned Nico) https://github.com/sailuh/kaiaulu/pull/162
• Malia's https://github.com/sailuh/kaiaulu/pull/163
• Nico's https://github.com/sailuh/kaiaulu/pull/167

[carlosparadis]

Thank you for making the references in a concise message. Then yes, this (your revisions to milestone 1 and any others of milestone 1) suffices for Nico's 22nd deliverable and/or ensuring your unit tests have similar formatting, as I exemplified between his code having rstudioapi (and also sprintf), and both yours and Malia have system2. He should ensure that across all Milestone 1 unit tests too.

What I will expect then from Nico is a commit to this Malia's pull request (via her fork) by the 22nd, finishing this, correct?

Thank you!

[nicoelee123]

@carlosparadis All the revisions seemed to be made and the tests all pass using the 'Build' -> 'Test Package'. Please let me know if there's something that I missed, otherwise it looks good to me!

[carlosparadis]

@nicoelee123 Could you make a commit changing NEWS.md for me mentioning under Documentation that unit tests were added? You need to commit to Malia's fork, so she has to grant you permission to her fork to do that.

Once you do that, the GitHub Actions I set will also test a few more things for us here. Make sure you merge her code to the more recent versions of the master branch, otherwise it will not trigger the tests.

[nicoelee123]

@carlosparadis I think I updated correctly and made changes. It says there are some checks queued up as well.

[carlosparadis]

@nicoelee123

If you click the "R-CMD-check" you can find the following in the text:

As you can see, the tests are failing. Are they passing on your machine? This is why I added the GitHub actions. It is basically simulating what would happen if I were to download and run the unit test on mine.

[carlosparadis]

Hi Nico,

I added one more GitHub Action for test-coverage. This should give you some more additional info on why the checks failed. Here is one more example on reading them. This is from the test-coverage:

It seems there is a problem in that region.

Please feel free to do more commits until the checks pass, and if you can't understand why ping me. Also, make sure you do one more merge across all the PRs for unit tests so all of them have the more recent GitHub actions. This should hopefully be my last commit you need to merge to the PRs.

Thanks.

[carlosparadis]

@nicoelee123

Let me know when you make the fixes for:

https://github.com/sailuh/kaiaulu/pull/162#pullrequestreview-1407031000

And can check that the same issues don't happen here and I will take another pass. I scanned quickly, and at least the unit test descriptions of this PR looks fine.

Thanks!

[nicoelee123]

@carlosparadis I made the changes and confirmed why the suppressWarnings were necessary.

[carlosparadis]

@nicoelee123 why were they necessary?

[nicoelee123]

The functions git_create_sample_log() and git_delete_sample_log() return a warning but it still successfully creates and deletes the sample log. I assumed based on Leilani's PR that you don't want warnings appearing, but if this warning is okay then I can remove the supressWarnings calls.

[carlosparadis]

No, it's fine. Keep them. I just wanted to make sure the reason is noted down! Thanks!

[codecov[bot]]

Codecov Report

Merging #163 (dcdc899) into master (882234f) will increase coverage by 2.65%. The diff coverage is n/a.

:exclamation: Current head dcdc899 differs from pull request most recent head 12a1c2d. Consider uploading reports for the commit 12a1c2d to get more accurate results

@@            Coverage Diff            @@
##           master    #163      +/-   ##
=========================================
+ Coverage    2.56%   5.22%   +2.65%     
=========================================
  Files          15      15              
  Lines        2145    2145              
=========================================
+ Hits           55     112      +57     
+ Misses       2090    2033      -57     

see 2 files with indirect coverage changes

[carlosparadis]

@malialiu there was one unit test that was failing for me, but my commit address it. However, after accepting the other unit tests I now have merge conflicts. Could fix these for me? Otherwise, this is ready to be accepted into master.