TST: protocols: Don't assume GitRepo runs a callable

[kyleam]

protocols.py distinguishes between external commands and callables.
To test the "callable" case, we create a GitRepo, assuming that this
will lead to gitpy.Repo.init() being executed through the runner.
This is problematic because it depends on internal details of GitRepo
and a reader has to dig into the GitRepo class to understand why we
expect a callable to be used.

And with -revolution, those internal details change. RevolutionGitRepo
overrides _create_empty_repo() and uses the external command 'git
init' as of 8b9d65a (ENH: Expose git-init options, create without
GitPython, 2018-10-29). As a result, these protocol tests fail if we
substitute RevolutionGitRepo for GitRepo.

Use os.mkdir as the callable to simplify these tests and prepare for
the absorption of RevolutionGitRepo into GitRepo.

[yarikoptic]

protocols.py distinguishes between external commands and callables.
To test the "callable" case, we create a GitRepo, assuming that this
will lead to gitpy.Repo.init() being executed through the runner.
This is problematic because it depends on internal details of GitRepo
and a reader has to dig into the GitRepo class to understand why we
expect a callable to be used.

I guess it was done so that we also check that we can protocol those GitRepo calls, which were our main calls of interest. So the "assumption" was on purpose, may be we could have just left a comment for the reader that it was intentional? So in principle we could/should've left it as such. But the fact of life is that we haven't developed more (e.g. we had an idea to protocol execution times) or used those protocols extensively, besides me using the one for protocolling interactions with git-annex, so pragmatically we could no longer assume that and change the test/code accordingly. OR introduced by revolution code could make use of this facility?

[kyleam]

This is problematic because it depends on internal details of GitRepo
and a reader has to dig into the GitRepo class to understand why we
expect a callable to be used.

I guess it was done so that we also check that we can protocol those GitRepo calls, which were our main calls of interest. So the "assumption" was on purpose

OK, I'll update my commit message so that I don't mischaracterize the intention, but ...

may be we could have just left a comment for the reader that it was intentional?

While a comment wouldn't have hurt, I think the current testing gets things backwards (which maybe shouldn't count for much since I didn't even know protocols existed until last week :). test_protocols.py should be about testing the module, not about testing that some other class adheres to a callable rather than an external command. GitRepo uses Runner (and thus protocols), not the other way around.

Plus, testing initialization of GitRepo doesn't do a thorough job of testing that the commands that GitRepo passes through the runner follow the expected "callable vs external"; it just tests the one runner call in _create_empty_repo.

So, if we want to enforce that a particular form, callable or external command, is used for a particular GitRepo method, I think we should document that expectation in the GitRepo class and have a test in tests_gitrepo.py that check this. (And the same would apply to AnnexRepo methods.)

So in principle we could/should've left it as such. But the fact of life is that we haven't developed more (e.g. we had an idea to protocol execution times) or used those protocols extensively, besides me using the one for protocolling interactions with git-annex, so pragmatically we could no longer assume that and change the test/code accordingly. OR introduced by revolution code could make use of this facility?

Sorry, I can't make out what your point is here, and how it relates to whether _create_empty_repo() passes a callable or external command to the runner.

[yarikoptic]

So in principle we could/should've left it as such. But the fact of life is that we haven't developed more (e.g. we had an idea to protocol execution times) or used those protocols extensively, besides me using the one for protocolling interactions with git-annex, so pragmatically we could no longer assume that and change the test/code accordingly. OR introduced by revolution code could make use of this facility?

Sorry, I can't make out what your point is here, and how it relates to whether _create_empty_repo() passes a callable or external command to the runner.

My point was that I am ok if we were to drop that testing of being able to protocol GitRepo calls if that would not interfere with protocoling interactions with git annex

[kyleam]

I'm lost. Is there something you're doing with protocols now that won't work when ["git", "init", ...] is passed to the runner instead of passing the callable gitpy.Repo.init?

[codecov]

Codecov Report

Merging #3145 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3145      +/-   ##
==========================================
- Coverage   90.74%   90.51%   -0.23%     
==========================================
  Files         248      248              
  Lines       32714    32715       +1     
==========================================
- Hits        29686    29612      -74     
- Misses       3028     3103      +75

Impacted Files	Coverage Δ
datalad/tests/test_protocols.py	100% <100%> (ø)	⬆️
datalad/interface/tests/test_unlock.py	92.59% <0%> (-7.41%)	⬇️
datalad/interface/ls.py	62.95% <0%> (-5.85%)	⬇️
datalad/support/s3.py	24.77% <0%> (-5.76%)	⬇️
datalad/support/tests/test_annexrepo.py	95.04% <0%> (-1.44%)	⬇️
datalad/downloaders/providers.py	95.41% <0%> (-1.38%)	⬇️
datalad/tests/utils.py	88.46% <0%> (-0.82%)	⬇️
datalad/downloaders/s3.py	57.36% <0%> (-0.78%)	⬇️
datalad/interface/add_archive_content.py	89.9% <0%> (-0.49%)	⬇️
datalad/support/sshconnector.py	84.31% <0%> (-0.34%)	⬇️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 697d8b0...2f2fb30. Read the comment docs.

[codecov]

Codecov Report

Merging #3145 into master will decrease coverage by 0.22%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3145      +/-   ##
==========================================
- Coverage   90.74%   90.51%   -0.23%     
==========================================
  Files         248      248              
  Lines       32714    32715       +1     
==========================================
- Hits        29686    29612      -74     
- Misses       3028     3103      +75

Impacted Files	Coverage Δ
datalad/tests/test_protocols.py	100% <100%> (ø)	⬆️
datalad/interface/tests/test_unlock.py	92.59% <0%> (-7.41%)	⬇️
datalad/interface/ls.py	62.95% <0%> (-5.85%)	⬇️
datalad/support/s3.py	24.77% <0%> (-5.76%)	⬇️
datalad/support/tests/test_annexrepo.py	95.04% <0%> (-1.44%)	⬇️
datalad/downloaders/providers.py	95.41% <0%> (-1.38%)	⬇️
datalad/tests/utils.py	88.46% <0%> (-0.82%)	⬇️
datalad/downloaders/s3.py	57.36% <0%> (-0.78%)	⬇️
datalad/interface/add_archive_content.py	89.9% <0%> (-0.49%)	⬇️
datalad/support/sshconnector.py	84.31% <0%> (-0.34%)	⬇️
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 697d8b0...2f2fb30. Read the comment docs.

[kyleam]

I've tweaked the commit message and added another commit with a new test, test_gitrepo.py:test_custom_runner_protocol:

Range-diff:

1:  f8f755599 ! 1:  38ac60a30 TST: protocols: Don't assume GitRepo runs a callable
    @@ -1,22 +1,17 @@
     Author: Kyle Meyer <kyle@kyleam.com>
     
    -    TST: protocols: Don't assume GitRepo runs a callable
    +    TST: protocol: Don't use GitRepo() to test callable
     
    -    protocols.py distinguishes between external commands and callables.
    -    To test the "callable" case, we create a GitRepo, assuming that this
    -    will lead to gitpy.Repo.init() being executed through the runner.
    -    This is problematic because it depends on internal details of GitRepo
    -    and a reader has to dig into the GitRepo class to understand why we
    -    expect a callable to be used.
    +    protocol.py distinguishes between external commands and callables.  To
    +    test the "callable" case, we create a GitRepo, assuming that this will
    +    lead to gitpy.Repo.init() being executed through the runner.  But in
    +    -revolution, RevolutionGitRepo overrides _create_empty_repo() and uses
    +    the external command 'git init' as of 8b9d65a (ENH: Expose git-init
    +    options, create without GitPython, 2018-10-29).  As a result, these
    +    protocol tests fail if we substitute RevolutionGitRepo for GitRepo.
     
    -    And with -revolution, those internal details change. RevolutionGitRepo
    -    overrides _create_empty_repo() and uses the external command 'git
    -    init' as of 8b9d65a (ENH: Expose git-init options, create without
    -    GitPython, 2018-10-29).  As a result, these protocol tests fail if we
    -    substitute RevolutionGitRepo for GitRepo.
    -
    -    Use os.mkdir as the callable to simplify these tests and prepare for
    -    the absorption of RevolutionGitRepo into GitRepo.
    +    Use os.mkdir to prepare for the absorption of RevolutionGitRepo into
    +    GitRepo.
     
      diff --git a/datalad/tests/test_protocols.py b/datalad/tests/test_protocols.py
      --- a/datalad/tests/test_protocols.py
-:  --------- > 2:  2f2fb308d TST: gitrepo: Add test for runner with non-default protocol