Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration tests for tools #43

Open
wants to merge 11 commits into
base: merge-2023-04-02
Choose a base branch
from
Open

Conversation

fblanchetNaN
Copy link
Collaborator

Integration tests for basic tools: setuptools, venv and git.

PR enabled to check all tests on the CI server, DO NOT MERGE yet.

Update versions for CI (GitHub Actions and pre-commit).

@fblanchetNaN fblanchetNaN force-pushed the test_tools branch 2 times, most recently from 9c7318d to 5c3bab9 Compare July 2, 2022 20:52
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2022

Codecov Report

Merging #43 (397edbc) into main (932e250) will increase coverage by 30.59%.
The diff coverage is 91.66%.

@@             Coverage Diff             @@
##             main      #43       +/-   ##
===========================================
+ Coverage   60.97%   91.56%   +30.59%     
===========================================
  Files          13       13               
  Lines         574      593       +19     
===========================================
+ Hits          350      543      +193     
+ Misses        224       50      -174     
Impacted Files Coverage Δ
incipyt/tools/git.py 90.69% <90.47%> (+90.69%) ⬆️
incipyt/__main__.py 85.33% <100.00%> (+85.33%) ⬆️
incipyt/_internal/templates.py 89.37% <0.00%> (+0.62%) ⬆️
incipyt/commands.py 93.10% <0.00%> (+24.13%) ⬆️
incipyt/_internal/sanitizers.py 76.92% <0.00%> (+76.92%) ⬆️
incipyt/tools/setuptools.py 97.91% <0.00%> (+97.91%) ⬆️
incipyt/signals.py 100.00% <0.00%> (+100.00%) ⬆️
incipyt/tools/base.py 100.00% <0.00%> (+100.00%) ⬆️
... and 2 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 932e250...397edbc. Read the comment docs.

Copy link
Member

@Julien00859 Julien00859 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be careful inside integration test, e.g. for that git config --global user.name, IMO we should mock those values before running the tests.

Please, don't store the truth inside of targz archives, it makes it impossible to read/diff on github

tests/test_tools/test_setuptools.py Show resolved Hide resolved
from incipyt.tools.setuptools import LINUX_MIN_PYTHON_VERSION


def make_archive():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing docstring, this test-file will be our integration-test showcase/template, it needs to be documented so people feel easy using it as a template for their own tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and please note: I understand nothing at this function, what the fuck is an isolated_filesystem? why the fuck you need to chdir and remove a targz file???

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where this function is used?

Comment on lines +33 to +35
shutil.unpack_archive(
pathlib.Path(__file__).parent / "setuptools.tar.gz", "archive"
)
Copy link
Member

@Julien00859 Julien00859 Jul 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just noticed that the "truth" is inside a tar.gz file tests/test_tools/setuptools.tar.gz. I think it is a bad idea, if one day we change something to the setuptools plugin, we'll have to adapt this tgz file but we won't have a clear diff of what changed.

Today's me want to read (inside github) what's inside that tgz file, future's me want to read a diff (inside github) of what changed inside that tgz file. Please don't store it as a tgz, just upload the whole folder as-is.



def diff_files(dircmp):
result = {str(pathlib.Path(dircmp.left) / f) for f in dircmp.diff_files}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fspath

from incipyt import project


def diff_files(dircmp):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what type is dircmp?

path
for path in result
if not any(
re.match(pattern, path) for pattern in [r".*/\.env/.*", r".*/dist/.*"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use or own .gitignore https://stackoverflow.com/a/22090594

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while I agree those need to be stored somewhere else (a list of globs, just as .gitignore does, is just fine), using incipyt own .gitignore seems ankward and error-prone to me

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, I'm not sure a 3rd party lib is really needed when we already have glob in the stdlib

Comment on lines 23 to 24
self._set_git_credential_email = False
self._set_git_credential_name = False
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git doesn't use credentials, github does. user.name and user.email are not credentials

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a name that starts with a verb should be a function, not a boolean


def post(self, workon):
"""Run `git add --all`.

:param workon: Work-on folder.
:type workon: :class:`pathlib.Path`
"""
if self._set_git_credential_email:
Copy link
Member

@Julien00859 Julien00859 Jul 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok now I understand.

So the commit message should be:

feat(git): user.name defaults to AUTHOR_NAME

Make sure there is no global git user.name and user.email: make sure
`git config --global user.name` is empty, same of `user.email`. Create a
project using git and setuptools: the wizard prompts you for an author
name and email as it couldn't determine them via git. When the project
is created, the author name and email are set inside `setup.cfg` but the
local git configuration still lacks an user.name and a user.email.

In this commit when no git user name or email is set globally and that
one was provided to the wizard, it automatically update the config of
the new git repository to set the user.name and the user.email to the
AUTHOR_NAME and AUTHOR_EMAIL.

@fblanchetNaN fblanchetNaN force-pushed the test_tools branch 2 times, most recently from 328f61f to 397edbc Compare July 3, 2022 16:35
@LeMinaw
Copy link
Collaborator

LeMinaw commented Jul 4, 2022

first integration tests failing on windows CI is due to non platform-agnostic path matching at testing.py:23

(diff is + {'my_project\\.env\\pyvenv.cfg'} which should be ignored)


def post(self, workon):
"""Run `git add --all`.

:param workon: Work-on folder.
:type workon: :class:`pathlib.Path`
"""
git_user_email_return = commands.run(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would isolate the "update git config" in a dedicated function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also amend the docstring

Comment on lines 112 to 121
commands.run(
[
"git",
"-C",
os.fspath(workon),
"config",
"user.email",
project.environ["AUTHOR_EMAIL"],
]
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I veto that.

Running $ git -C {} config user.email {} shouldn't expand over 9 lines of code.

def git(args, workon, **kwargs):
    args = ["git", "-C", os.fspath(workon), *args]
    return commands.run(args, **kwargs)

git = partial(git, workon=workon)

@Julien00859 Julien00859 changed the base branch from main to 20230402 April 2, 2023 13:13
fblanchetNaN and others added 11 commits April 2, 2023 15:20
This work include two new utilities, one to create a `StringTemplate`
out of a file located inside of the new `/templates` directory, the
other to copy a template file directly to the project.
A software license is a file stored at the root of a project which
explain what it is allowed to do with the project source code and under
what conditions. e.g. a project is qualified as "open-source" when it is
published with a license that allows to distribute and modify the
project source code.

Until now the license file generated by incipyt was a simple copyright
notice which made it explicit that the current work was the intellectual
property of its owner. Under internationnal laws, this forbid anyone
from modifying the work without the express authorization of the author.

In this work, we added a way so that the user can choose an open-source
license instead of the copyright notice. We added `--license <license>`
to the CLI arguments and bundled common open-source licenses such as MIT
and GPL.

The previous behavior, to simply include a copyright notice, is still
possible thanks to `--license copyright`.
See https://docs.pytest.org/en/latest/how-to/fixtures.html#yield-fixtures-recommended
pytest recommends using a yield construction to setup and teardown
environments for tests. Here we use it to clean project.environ and
project.structure to provide reliable initial project in any case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants