Have memote.utils.is_modified() return False if file was deleted.

[jonovik]

memote history will fail on a commit that deleted the model file.

Running the test suite for commit '8daf...'.
Traceback (most recent call last):
...
  File "C:\NMBU\Miniconda3\envs\cbm\lib\site-packages\click\core.py", line 535, in invoke
    return callback(*args, **kwargs)
  File "c:\git\digisal\memote\memote\suite\cli\runner.py", line 393, in history
    blob = cmt.tree[model]
  File "C:\NMBU\Miniconda3\envs\cbm\lib\site-packages\git\objects\tree.py", line 298, in __getitem__
    return self.join(item)
  File "C:\NMBU\Miniconda3\envs\cbm\lib\site-packages\git\objects\tree.py", line 225, in join
    item = tree[token]
  File "C:\NMBU\Miniconda3\envs\cbm\lib\site-packages\git\objects\tree.py", line 298, in __getitem__
    return self.join(item)
  File "C:\NMBU\Miniconda3\envs\cbm\lib\site-packages\git\objects\tree.py", line 244, in join
    raise KeyError(msg % file)
KeyError: "Blob or Tree named 'export' not found"

(where export referred to my model file export/model.xml)

This is because memote.utils.is_modified just tests for path in commit.stats.files, whereas a deletion shows up as {'insertions': 0, 'deletions': N, 'lines': N} where N is the number of lines in the file.

This example shows a file being created, modified, untouched, deleted (in reverse chronological order):

>>> [commit.stats.files for commit in repo.iter_commits()]
[{'test_file.txt': {'insertions': 0, 'deletions': 1, 'lines': 1}},
 {},
 {'test_file.txt': {'insertions': 1, 'deletions': 1, 'lines': 2}},
 {'test_file.txt': {'insertions': 1, 'deletions': 0, 'lines': 1}}]

Since we cannot run tests on a nonexistent model file, it is more robust to skip those commits. I have modified memote.utils.is_modified and added a test which fails in 276630f but passes in my pull request.

[jonovik]

I think the tests failing are due to the typo with the path variable. So hopefully it's an easy fix. Thanks a lot for spotting this and providing a solution 😃

That is curious. The tests pass on my system; see below.

Just to be clear: test_for_utils.py::test_is_modified_deleted was designed to fail in the current memote, demonstrating the problem. And with my modifications to memote.utils.is_modified, it passes.

I was waiting for continuous integration to verify that it also works under linux, but am not intimately familiar with Travis. Are the "Some checks were not successful" referring to my commit, or to your proposed changes?

(cbm) C:\git\digisal\memote\tests>pytest -v test_for_utils.py
======================================================================================================== test session starts ========================================================================================================
platform win32 -- Python 3.6.8, pytest-4.4.1, py-1.8.0, pluggy-0.9.0 -- c:\nmbu\miniconda3\envs\cbm\python.exe
cachedir: .pytest_cache
metadata: {'Python': '3.6.8', 'Platform': 'Windows-10-10.0.17763-SP0', 'Packages': {'pytest': '4.4.1', 'py': '1.8.0', 'pluggy': '0.9.0'}, 'Plugins': {'metadata': '1.8.0', 'html': '1.20.1.dev1+g27b6e77'}, 'JAVA_HOME': 'C:\\Program Files\\Java\\jdk1.8.0_112\\jre'}
rootdir: C:\git\digisal\memote, inifile: setup.cfg
plugins: metadata-1.8.0, html-1.20.1.dev1+g27b6e77
collected 11 items

test_for_utils.py::test_register_with[one-one] PASSED                                                                                                                                                                          [  9%]
test_for_utils.py::test_register_with[two-two] PASSED                                                                                                                                                                          [ 18%]
test_for_utils.py::test_register_with[three-three] PASSED                                                                                                                                                                      [ 27%]
test_for_utils.py::test_register_with[four-four] PASSED                                                                                                                                                                        [ 36%]
test_for_utils.py::test_annotate[notes0-one-One line.] PASSED                                                                                                                                                                  [ 45%]
test_for_utils.py::test_annotate[notes1-two-Two lines.\n    Why?\n    ] PASSED                                                                                                                                                 [ 54%]
test_for_utils.py::test_annotate[notes2-three-\n    Three lines.\n    Why?\n    ] PASSED                                                                                                                                       [ 63%]
test_for_utils.py::test_annotate[notes3-four-\n    Fourth summary.\n\n    A proper description.\n\n    Returns\n    -------\n    None\n\n    ] PASSED                                                                          [ 72%]
test_for_utils.py::test_annotate[notes4-one-] FAILED                                                                                                                                                                           [ 81%]
test_for_utils.py::test_show_versions PASSED                                                                                                                                                                                   [ 90%]
test_for_utils.py::test_is_modified_deleted PASSED

[jonovik]

@Midnighter , I'm sorry. I had forgotten to commit my latest changes. Hang on and we'll see if it works out...

[jonovik]

@Midnighter , I'm sorry. I had forgotten to commit my latest changes. Hang on and we'll see if it works out...

Please disregard that comment. The uncommitted changes were a reversal of memote.utils while keeping my tests.test_for_utils, which I had used to verify that my test was indeed failing before.

After reinstalling memote from that version (with my test but the buggy utils.py) with pip install --upgrade c:/git/digisal/memote I am getting the failure that I expected; see below.

However, I note that the failure is different than what Travis CI is showing: E assert (False, False, True, True) == (False, False, False, False)
versus my E assert (False, False, True, True) == (True, False, True, True).

So maybe it is gitpython behaving differently under Windows (this is in the usual CMD shell, not git bash). I'll push your suggestion of including an unrelated file in the mock commit as you proposed, in case that makes behaviour more reproducible.

(cbm) C:\git\digisal\memote>pytest -v tests/test_for_utils.py
======================================================================================================== test session starts ========================================================================================================
platform win32 -- Python 3.6.8, pytest-4.4.1, py-1.8.0, pluggy-0.9.0 -- c:\nmbu\miniconda3\envs\cbm\python.exe
cachedir: .pytest_cache
metadata: {'Python': '3.6.8', 'Platform': 'Windows-10-10.0.17763-SP0', 'Packages': {'pytest': '4.4.1', 'py': '1.8.0', 'pluggy': '0.9.0'}, 'Plugins': {'metadata': '1.8.0', 'html': '1.20.1.dev1+g27b6e77'}, 'JAVA_HOME': 'C:\\Program Files\\Java\\jdk1.8.0_112\\jre'}
rootdir: C:\git\digisal\memote, inifile: setup.cfg
plugins: metadata-1.8.0, html-1.20.1.dev1+g27b6e77
collected 11 items
...
tests/test_for_utils.py::test_show_versions PASSED                                                                                                                                                                             [ 90%]
tests/test_for_utils.py::test_is_modified_deleted FAILED                                                                                                                                                                       [100%]

============================================================================================================= FAILURES ==============================================================================================================
...
c:\nmbu\miniconda3\envs\cbm\lib\site-packages\memote\utils.py:119: ValueError
_____________________________________________________________________________________________________ test_is_modified_deleted ______________________________________________________________________________________________________

mock_repo = ('test_file.txt', <git.Repo "C:\Users\jonvi\AppData\Local\Temp\pytest-of-jonvi\pytest-3\memote-mock-repo0\.git">)

    def test_is_modified_deleted(mock_repo):
        """Don't report file deletion as modification."""
        relname, repo = mock_repo
        # File history (newest first): deleted, unchanged, modified, created
        # Don't report file deletion as "modification"
        # for purposes of running memote tests on the file.
        want = False, False, True, True
        # Convert to tuple so we can compare want == got,
        # which pytest will introspect helpfully if the assertion fails.
        got = tuple(utils.is_modified(relname, commit)
                    for commit in repo.iter_commits())
>       assert want == got
E       assert (False, False, True, True) == (True, False, True, True)
E         At index 0 diff: False != True
E         Full diff:
E         - (False, False, True, True)
E         + (True, False, True, True)

tests\test_for_utils.py:158: AssertionError
================================================================================================ 2 failed, 9 passed in 3.80 seconds =================================================================================================

[jonovik]

So maybe it is gitpython behaving differently under Windows (this is in the usual CMD shell, not git bash). I will zip up my mock repo and email it to you. I'll also push your suggestion of including an unrelated file in the mock commit as you proposed, in case that makes behaviour more reproducible.

@Midnighter Adding an unrelated file does indeed give the same error that you are seeing. I'll look into why that would break the logic of my is_modified(). I'll ping you when I have a solution.

[jonovik]

@Midnighter You were right about the path typo. What baffles me is how this could still be passing on my computer. Hope everything is okay now. (Tests pass on my system, for what that's worth 8-)

[jonovik]

I see that the build is failing on Python 3.5 and presumably earlier versions due to something something LocalPath. I'll have another look at the git repo mockup I was imitating.

[Midnighter]

Looking at the Travis output, I think you just need a small change in the fixture.

[jonovik]

Looking at the Travis output, I think you just need a small change in the fixture.

Yes, I think I know what to do (convert LocalPath to str). Just waiting for conda to make me a 3.5 test environment.

[codecov[bot]]

Codecov Report

Merging #662 into develop will decrease coverage by 0.09%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop     #662     +/-   ##
==========================================
- Coverage    84.99%   84.89%   -0.1%     
==========================================
  Files           59       59             
  Lines         3258     3264      +6     
  Branches       436      437      +1     
==========================================
+ Hits          2769     2771      +2     
- Misses         401      404      +3     
- Partials        88       89      +1

Impacted Files	Coverage Δ
memote/utils.py	75.78% <100%> (+1.63%)	:arrow_up:
memote/suite/tests/test_thermodynamics.py	88.88% <0%> (-11.12%)	:arrow_down:
memote/suite/cli/runner.py	51.81% <0%> (-0.52%)	:arrow_down:

---

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 276630f...bc8ee46. Read the comment docs.

[codecov[bot]]

Codecov Report

Merging #662 into develop will decrease coverage by 0.09%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           develop     #662     +/-   ##
==========================================
- Coverage    84.99%   84.89%   -0.1%     
==========================================
  Files           59       59             
  Lines         3258     3264      +6     
  Branches       436      437      +1     
==========================================
+ Hits          2769     2771      +2     
- Misses         401      404      +3     
- Partials        88       89      +1

Impacted Files	Coverage Δ
memote/utils.py	75.78% <100%> (+1.63%)	:arrow_up:
memote/suite/tests/test_thermodynamics.py	88.88% <0%> (-11.12%)	:arrow_down:
memote/suite/cli/runner.py	51.81% <0%> (-0.52%)	:arrow_down:

---

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 276630f...bc8ee46. Read the comment docs.

[codecov[bot]]

Codecov Report

Merging #662 into develop will increase coverage by 2.47%. The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #662      +/-   ##
===========================================
+ Coverage    82.54%   85.01%   +2.47%     
===========================================
  Files           49       59      +10     
  Lines         2601     3264     +663     
  Branches       424      437      +13     
===========================================
+ Hits          2147     2775     +628     
- Misses         368      401      +33     
- Partials        86       88       +2

Impacted Files	Coverage Δ
memote/utils.py	75.78% <100%> (ø)
src/memote/suite/cli/__init__.py
src/memote/experimental/growth.py
src/memote/support/sbo.py
src/memote/support/consistency.py
src/memote/suite/results/__init__.py
src/memote/support/matrix.py
src/memote/experimental/__init__.py
src/memote/experimental/essentiality.py
src/memote/__init__.py
... and 99 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 7618513...1841901. Read the comment docs.

[Midnighter]

Ahh, sorry @jonovik. @ChristianLieven reworked a lot of the git interaction logic recently because gitpython doesn't fully work on Windows. If you want we can take it from here. Thank you for the patch and test so far!

[jonovik]

Ahh, sorry @jonovik. @ChristianLieven reworked a lot of the git interaction logic recently because gitpython doesn't fully work on Windows. If you want we can take it from here. Thank you for the patch and test so far!

@Midnighter Please do take it from here.

@ChristianLieven As for git on Windows, I've found that repo.git.commit works where repo.index.commit fails in getting hooks to work on Windows. I am eagerly awaiting your reworking. Here are a couple of my own notes, in case they may be useful (commit message from a private branch):

Use repo.git.commit instead of repo.index.commit for hooks to work on Windows.

Given repo = git.Repo(...), Gitpython's repo.index.commit(message) uses os.subprocess to start .git/hook/pre-commit. However, Windows does not recognize this text file as a Python script, and so this fails with OSError: [WinError 193] %1 is not a valid Win32 application

Using repo.git.commit("-m", message) instead invokes the git executable instead, which (I think) parses the shebang of .git/hook/pre-commit and does the right thing.

[Midnighter]

Made some more modifications in order to standardize git use across the project. Thank you very much for this addition @jonovik and my sincere apologies for dragging this out so long. It fails on Travis for Python 2.7 but works for me locally. Since we are anyway dropping support for 2, I will merge it here.