add test using git

[mawillcockson]

This adds a few pytest fixtures that set up a git repository using git.

I used this stackoverflow answer to isolate git using these environment variable:

• GIT_CONFIG_GLOBAL
• GIT_CONFIG_NOSYSTEM
• HOME
• GIT_AUTHOR_EMAIL
• GIT_AUTHOR_NAME
• GIT_AUTHOR_DATE
• GIT_COMMITTER_EMAIL
• GIT_COMMITTER_NAME
• GIT_COMMITTER_DATE

Instead of using GIT_DIR and GIT_WORK_TREE, I added another fixture that forms a closure around git, and calls it's -C command-line option, pointing to the temporary repository.

I'm not familiar with testing git, so I'm not sure if these measures are sufficient.

The tmp_git_repo fixture initializes the temporary repository with an empty commit. This isn't strictly necessary, it just makes some of the possible tests require less setup.

The tmp_project fixture copies the sample module from ./tests/samples/module1_toml to the repository and commits the files.

A test is added for #345 as an example of how to use the new fixtures.

I wanted to be able to run tox and pytest targeting or excluding only the tests that depend on git being installed:

tox -- -m needgit

or skipped:

tox -- -m "not needgit"

I kept getting confused myself, so both needgit and needsgit work.

Currently this is selected based on the test function name including needgit or needsgit, and I can understand if that's a bit too magical.

The tests are skipped if shutil.which("git") returns nothing, though I did not test this.

---

Possible changes

Would it make more sense to have tmp_project behave similarly to the existing copy_sample fixture?

It could take as an argument the sample module and return a contextmanager-wrapped iterator that copies the named sample module files into the git repository and commits the changes:

def test_example(copy_to_repo):
    with copy_to_repo("module1_toml") as repo:
        ...

[takluyver]

Thanks! This looks like a good start. I don't know exactly what's needed to isolate git either, so we may as well try it and fix things as they come up. :smile:

The thing with the test names is indeed a bit more magic than I like. Let's just use an explicit @pytest.mark.needsgit marker. I'd also prefer to have just the one name - I can see the potential for mistakes, but it feels kind of messy to solve that by having both. We can use pytest's --strict-markers option to check that the markers in the code are all correct.

I think having the fixtures automatically copy one working sample is a decent starting point for now. But I might try to separate the layer for calling git (including all those environment variables to make it consistent) from the layer that prepares a specific repository. We could always refactor that later when we want more flexibility, though.

Lastly, it looks like the tests are failing on Windows (Appveyor CI). If needs be, we can skip it on Windows and only have the new tests on Linux for now, but if possible it would be good to figure it out.

[mawillcockson]

Thanks for being welcoming to contributions, and for giving very friendly feedback.

Let's just use an explicit @pytest.mark.needsgit marker I'd also prefer to have just the one name

✔️ https://github.com/takluyver/flit/pull/445/commits/7430aee7e4f60a76a269e09e234136245e26bb63

I might try to separate the layer for calling git (including all those environment variables to make it consistent) from the layer that prepares a specific repository.

Currently, the tmp_git_repo fixture sets and removes environment variables, and makes a temporary repository. I can separate those two if you'd prefer.

The tmp_project fixture requests tmp_git_repo and copies the sample files into the repository.

Is this separation of concerns sufficient? What did you have in mind?

---

Lastly, it looks like the tests are failing on Windows (Appveyor CI). If needs be, we can skip it on Windows and only have the new tests on Linux for now, but if possible it would be good to figure it out.

It looks like the tests are working already! 😁

Summary of the bug: git returns file paths separated by / regardless of the platform.

In the test this pr adds (starting from this line):

• flit.vcs.list_untracked_deleted_files() returns dist/module1-0.1.tar.gz
• flit.sdist.include_path() checks if this string starts with dist\
• A VCSError is thrown because the dist/ folder is not excluded

~~I would be happy to open an issue and PR for a quick fix.~~ ✔️ #447

My personal preference with Python is to pathlib-ify everything dealing with file paths, and I'd be happy to open another PR to do this.