Update code coverage documentation

[apcraig]

PR checklist

• Short (1 sentence) summary of your PR:
  Update code coverage documentation
• Developer(s):
  apcraig
• Suggest PR reviewers from list in the column to the right.
• Please copy the PR test results link or provide a summary of testing completed below.
  No testing done, this diff just updates documentation and adds a reference script
• How much do the PR code changes differ from the unmodified code?
  • bit for bit
  • different at roundoff level
  • more substantial
• Does this PR create or have dependencies on Icepack or any other models?
  • Yes
  • No
• Does this PR add any new test cases?
  • Yes
  • No
• Is the documentation being updated? ("Documentation" includes information on the wiki or in the .rst files from doc/source/, which are used to create the online technical docs at https://readthedocs.org/projects/cice-consortium-cice/. A test build of the technical docs will be performed as part of the PR testing.)
  • Yes
  • No, does the documentation need to be updated at a later time?
    • Yes
    • No
• Please provide any additional information or relevant details below:

Testing of the latest code coverage changes looks good. This PR just updates the documentation for carrying the testing out. In particular, creation of the special test_cice_icepack repository.

Over the next week or two, I'll work with @rallard77 (if willing) to make sure the process works for other consortium members.

Finally, a separate task is to review the coverage reports and improve testing.

[eclare108213]

Thanks @apcraig, I'm SO GLAD your've gotten this working. Considering our effort to reduce redundancy, what is the purpose of having the scripting commands both in the documentation and in the script itself? If I understand correctly, the script can't be run as a script. Is that true?

[apcraig]

@eclare108213, you are right that the documentation and script are redundant. I was undecided about which approach to take so did both, but I knew this would not be ideal. Especially as I have been a proponent of reducing redundancy. I have removed the script again, and think the documentation is more than adequate at this point. The other option would be to leave the script but have the documentation point to it. If anyone feels this is a better option, let me know. Thanks!

[eclare108213]

Yes, thanks for the chuckle. If the script can't actually be run as it is, then it's probably better not to have it as a script. If it can be fixed to run without manual intervention, then I'd recommend keeping the script and pointing the documentation to it. My $0.02.

[apcraig]

OK, I just updated things again. I have added a working script and removed the redundant documentation. There were some challenges with the script, but I think reasonable compromises have been made. You have to run the script interactively because it will ask for git login information when you push to the test_cice_icepack repository. Once this PR is merged, I will coordinate testing with others.

[eclare108213]

At some point, some of the rest of us ought to try to do this, but it sounds like this PR is an interim step, getting there.

[phil-blain]

if the clone is done using SSH ([email protected]:apcraig/Test_CICE_Icepack.git) instead of HTTPS, the script will not ask for a password for git push as long as the SSH keys are correctly configured

[phil-blain]

Also, if we want to this to be easier to setup for others, I think the username (here apcraig) should be a script parameter, or at least an environment variable.

But as I wrote in #89 (comment), I think it would be more "official" if the "test_cice_icepack" repo (or whatever we call it) was under the CICE-Consortium organization no ?

[apcraig]

Thanks @phil-blain. We could use ssh, but I'm not sure it's worth setting up the ssh keys for occasional testing. Putting in a username and password doesn't seem like a large burden or hurdle.

I have given a few people access to the test_cice_icepack repo. You should be able to push to that repo using your own username/password. Or are you suggesting that the "organization" name be a variable? My idea is that a handful of us will have access to the repo under "apcraig", and we'll use it for now. I don't see any value in setting up the equivalent repo under multiple users. If we did that, we'd end up needing to test/change a few other things including the codecov location and the token string that connects github and codecov. It seems just easier to give a subset of folks write access to my test_cice_icepack repo. It's just a temporary repo and if it's get trashed, it's not a big deal. Every use basically removes all files and adds them all back again anyway.

I replied in #89 that I would prefer to keep the test_cice_icepack repo hidden and out of the CICE-Consortium. My hope is that it's temporary (until codecov supports submodules although I don't have a lot of optimism), and I prefer the fake repo not be visible to the community in general, especially since it's not part of the standard workflow and testing. I think it can only create confusion in the community. It would be easy to migrate test_cice_icepack to the consortium org if others felt it was better there though.

[phil-blain]

Also, I'm not convinced we need a separate repository for the coverage to work with icepack.

We could maintain a separate branch in CICE-Consortium/CICE, call it something like codecov, and configure codecov to report coverage for this branch. This branch would be periodically automatically updated from master, but with icepack as a subdirectory instead of a submodule.

Something like this would work to initially create the branch (first time setup) :

# Create the branch from `master`
git checkout --recurse-submodules -b codecov master
# Remove icepack as a submodule, but add it back as a directory
mv icepack/ icepack-cp       && \ # keep a copy of the files
git submodule deinit icepack && \ # deinit the submodule
git rm icepack               && \ # remove the submodule and stage the removal
mv icepack-cp icepack        && \ # put the files back in place
rm icepack/.git              && \ # remove the gitfile
git add icepack              && \ # stage the files
git commit -m "codecov: create branch from $(git rev-parse --short master)"

Then, when we want to update the codecov branch:

# checkout the `codecov` branch
git checkout --recurse-submodules -f codecov
# prepare to create a merge, do not commit yet, and keep all files as-is 
git merge -s ours --no-commit master
# overwrite all files with the content of `master`, recursively in submodules
git read-tree -m -u --recurse-submodules master 
# Remove icepack as a submodule, but add it back as a directory
mv icepack/ icepack-cp       && \ # keep a copy of the files
git submodule deinit icepack && \ # deinit the submodule
git rm -f icepack            && \ # (force) remove the submodule and stage the removal
mv icepack-cp icepack        && \ # put the files back in place
rm icepack/.git              && \ # remove the gitfile
git add icepack              && \ # stage the files
git commit -m "codecov: merge master at $(git rev-parse --short master)"

This would create a history looking like this:

$ git log --oneline --decorate --all --graph -11
*   3ee36bc (codecov) codecov: merge master at b5134ad
|\  
| * b5134ad (HEAD -> master, origin/master, origin/HEAD) fixing bulletin board link (#391)
| * 3e5f693 report_results.csh: add html anchors to hashes and machines (#388)
| * 1071c7b MPI: use mpi module instead of Fortran include statement (#389)
| * 73fb4d0 update icepack (#387)
| * 41b4b44 Update Icepack tracer calls for recent refactor (#385)
* | 58d1c0c codecov: merge master at d964a74
|\| 
| * d964a74 Update Icepack interfaces consistent with recent changes in Icepack (#379)
| * 1adef75 Updated dmi drivers according to standalone (#383)
* | ad2e2ab codecov: create branch from b5134ad
|/  
* 29b99b6 CICE: Floe size distribution  (#382)

Note that the history of master stays linear, but the history of codecov has merges to record when the branch is updated. I think this is a appealing way to do this.

[apcraig]

Thanks @phil-blain, lots of good ideas. What I've implemented is certainly not the most sophisticated, partly because of my knowledge of git and partly because of my tendency to be a little more conservative in my approach. But I did consider a number of approaches. One approach I considered was to checkout CICE, remove the icepack submodule piece (similar to what you outline above with deinit) and push directly to test_cice_icepack. I viewed this as more risky than checking out test_cice_icepack and then copying in the latest files simply because of the definition of "origin" and the risk that you push to origin accidently if you are working in a CICE sandbox.

What is proposed above is similar to my rejected option but we would now be pushing directly to the CICE branch. I view this approach as even more risky than the option I rejected. This latest proposal requires that changes be pushed directly into the CICE repository without benefit of a PR. We will also create a branch in CICE that will be fundamentally different than master in terms of the submodule. I think this could be confusing to users. Finally, only a very limited number of people have write access to the CICE repository. By actually leveraging a separate repository, we can allow more people to contribute to the code coverage testing. Finally, I don't think the history of the test_cice_icepack repo/branch is critical. It's nice to know what version of CICE it's aligned to, and I admit that tracking the history of CICE in test_cice_icepack better would be helpful. But I still view this tool as something that is run occasionally (a few times a year), and the main thing is that when we test, we're testing the current CICE code. If we decide we want to automate the code coverage testing and run it weekly (for instance), then a different approach might be appropriate.

I am certainly open to other approaches. I also understand that I tend to be fairly conservative about these things and am happy to move in a different direction. I guess I would argue that we should see whether what we have now is useful, working, and robust. We should use the information from the code coverage tool to improve our test coverage over the next several months. And we should continue to periodically run code coverage test manually to reassess the situation. The current implementation is relatively straight-forward to carry out, largely hidden from the community, and is easy to drop if it turns out to be less useful than we hoped. If we gain experience and decide we do want automated weekly testing or need to update the implementation for other reasons, then we can certainly do that in the future. Anyway, that's what I propose at this point, but as I said, I'm open to other ideas.