add tests

[meeuw]

Hi! I noticed #48 and thought I could help.

It was quite a challenge (as the Python scripts prevent loading as a module) but I think this could work.

You can run this test using:

python -mvenv venv
venv/bin/pip install pytest-cov
venv/bin/pytest --cov --cov-report=html

I only have some trouble with the last test (test_missing), I cannot trigger the args.missing if condition.

Do you remember how to cover/reproduce this scenario?

This is stilll work in progress and I'd love to hear your feedback. I tried hard to be able to get code coverage, if you don't care about that it might be simpler not to use pytest and rewrite this into a simple test script.

I now focused on git-restore-mtime but this can (and should) be extended to the other scripts.

[MestreLion]

This is an AMAZING initiative, thank you so much!

A few answers / guidelines:

• Don't bother with the other tools, at least for now. They're shell / bash scripts, not Python, so it would require an entirely different testing framework. Besides, restore-mtime is the only one that truly matters, and in the future I might shove all the others in a tools subdir or move them to an entirely different repository.
• Triggering missing files is tricky, it requires not using --merge and a repository with files created in a merge commit and never modified again. I remember the Linux Kernel had 2 such files a few years ago, but this might not be the case anymore. Besides, after the new merge handling and the git log change, it's very possible that now my tool catches all files automatically, without ever requiring a 2nd pass.
• Code coverage is an important metric, specially now that this project is getting more visibility. I'm glad you took the harder route, would be amazing to keep this feature.
• Sorry for the __main__ guard, I realize how challenging it was to import it. Maybe we could simply move it to the end before the last try block that calls main() like most scripts? The code was never meant to be used as a library, but I'm not sure if there are any benefits on the current over-restrictive guard.

[meeuw]

Thanks for your answer and glad you like it!

I'll test it with the Linux Kernel now and let you know, my guess is also that this is solved with using git log.

The __main__ guard isn't the only hurdle, the script also doesn't have a .py filename extension and much is initialized at the "module" level.

The pythonic way would be to have a main function and a stub which calls the script, but this all also seems to be an overkill.

[meeuw]

Thanks for the suggestion, it seems that merge commits with renames still require this.

For example this commit: torvalds/linux@1a9239b

Even github seems to have some trouble showing this commit 😄

I'll see if I can create a small test scenario for this.

[meeuw]

Okay, it was pretty simple; just add something in a merge bubble.

I also added some tests for when the worktree is dirty (and force).

Now the coverage is:

Name                             Stmts   Miss  Cover
----------------------------------------------------
git-restore-mtime                  259     36    86%
test/test_git_restore_mtime.py     138      0   100%
----------------------------------------------------
TOTAL                              397     36    91%

I guess also some Windows and NetBSD tests would be nice, but I'd rather do that another time (in another PR).

Do you think these tests should also be run for CI in a GitHub Action?

[MestreLion]

Intentional or debug leftover?

[meeuw]

whups, removed in 6354f4e

[MestreLion]

The __main__ guard isn't the only hurdle, the script also doesn't have a .py filename extension and much is initialized at the "module" level.

Executables in *nix tend not to have extensions (or underscores). Many python projects get away with this by having a (usually shell) wrapper script that simply calls venv/bin/python mymodule. But, as you mentioned, an additional file would be overkill for a single-script though, so I just used a shebang.

To mitigate that and ease development I keep a local git_restore_mtime.py symlink to it, as hinted by the .gitignore entry.

As for the module-level initialization, it's basically the command-line parsing and subsequent function reassignments based on it. And yeah, it can see how this makes argument mocking very hard, sorry. Well, one could move all this to the beginning of main, but that would require a few global statements for reassigning touch() and isodate(). Not very pythonic either, but maybe worth it? And also give main() input parameters to ease argument mocking.

Those, along with a more friendly __main__ guard, might be a good refactoring for another PR. Go for it if you want to!

[meeuw]

It's great to read your open minded to changes to the current script to improve testability!

I'm not sure what's best, I really like the current simplicity. I give it some thoughts.