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

Devops: Add tox environment that runs tests with sqlite backend #6450

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

agoscinski
Copy link
Contributor

@agoscinski agoscinski commented Jun 5, 2024

The problem with the current tox test environments is that they do require psql database which require a service running in the background. With the merge of #6425, we can use the sqlite backend to run tests more encapsulated with less dependencies on global configurations.

I still need to figure out how to set up a ssh config in a virtual-environment-friendly way. We could just run the .github/workflow/setup_ssh.sh file, but then it messes too much with existing ssh configuration. One could add an include in ${HOME}/.ssh/config to include another config file that is stored locally in the tox environment.

Include ${AIIDA_DEV_TOX_ENVIRONMENT}/custom_ssh_config

This just adds one line to the user config, that only works inside the tox environment. So does not have any effect outside of the environment to the functionality of ssh.

Copy link

codecov bot commented Jun 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.83%. Comparing base (ef60b66) to head (955e400).
Report is 112 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6450      +/-   ##
==========================================
+ Coverage   77.51%   77.83%   +0.33%     
==========================================
  Files         560      566       +6     
  Lines       41444    41983     +539     
==========================================
+ Hits        32120    32675     +555     
+ Misses       9324     9308      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielhollas
Copy link
Collaborator

@agoscinski FYI One another improvement that I wanted to do but did not get to is to make graphviz dependency optional i.e. to automatically skip tests that need it if it is not available. We already do this for other external optional dependencies (e.x. xcrysden et al).

@sphuber
Copy link
Contributor

sphuber commented Jul 17, 2024

@agoscinski what is the status for this PR?

@agoscinski agoscinski force-pushed the devops/tox-presto branch 9 times, most recently from 952f11d to 5b8a0cb Compare August 6, 2024 10:38
@agoscinski
Copy link
Contributor Author

agoscinski commented Aug 6, 2024

I wanted to allow that the developer can set an ssh key that is not ~/.ssh/id_rsa. Some tests however rely on ssh_auto which determines the ssh key automatically which basically just checks for ~/.ssh/id_rsa and other encryption protocols. Changing the default .ssh folder is I think not possible with tox.ini (at least not in a convenient way). For these ssh_auto tests I would monkey patch the SshAutoTransport instance to set the ssh key only in case AIIDA_PYTEST_SSH_KEY is set. In the CI we still can use the default key, so this monkey patching only effects tests that run locally and set this environment (so I would remove my current changes in the .github/* files).

There are two ssh_key fixtures (tools/pytest_fixtures/orm.py and manage/tests/pytest_fixtures). I feel like this should be cleaned up (removing one), but I did not want to touch it for now in this PR. I am generally not sure about the usefulness of creating the ssh key, since we need to add it to authorized keys so we are able to ssh to localhost. At least my understanding of the tests that uses the ssh_key fixture is that the generated ssh key is rejected and then it fall backs to the default one. Keeping the logic that generates a temporary key gives the wrong impression that the generated ssh key is really used. In my opinion we should do something like this

@pytest.fixture(scope='session')
def ssh_key(tmp_path_factory) -> t.Generator[pathlib.Path, None, None]:
    import os
    if (ssh_key_path := os.environ.get('AIIDA_PYTEST_SSH_KEY')) is not None:
        yield pathlib.Path(ssh_key_path)
    else:
        yield None # fallback to default key

We could move the logic that generates a new key to a new fixture, but I have the impression that it is not really used anywhere, so why bother. Since running these tests requires some global configuration, I would also mark these tests (in the pytest sense) with something like ssh_localhost.

@agoscinski agoscinski requested a review from sphuber August 6, 2024 12:08
@agoscinski agoscinski self-assigned this Aug 6, 2024
@agoscinski agoscinski requested a review from khsrali August 6, 2024 12:08
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Thanks @agoscinski left a few comments

set -ev

mkdir -p ${PWD}/.ssh
Copy link
Contributor

Choose a reason for hiding this comment

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

I take it this is necessary because you do ${{ github.workspace }} in the GHA workflow. But wouldn't it be better to use the temporary file system or ${HOME} as well there? Or do those options not work?

Copy link
Contributor Author

@agoscinski agoscinski Sep 10, 2024

Choose a reason for hiding this comment

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

One can also put the key to some tmp folder, but then one has to add each generated key to authorized_keys which fills up the file over time (as the tmp folder will be deleted). Since the authorized_keys is a user file that one cannot just be deleted I rather don't do this. So I did the intermediate solution and put it in the project folder. ${HOME} would also work, maybe this is even better since a project folder might be also deleted frequently.

src/aiida/manage/tests/pytest_fixtures.py Outdated Show resolved Hide resolved
src/aiida/tools/pytest_fixtures/orm.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
tests/tools/pytest_fixtures/test_configuration.py Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
set -ev

ssh-keygen -q -t rsa -b 4096 -N "" -f "${HOME}/.ssh/id_rsa"
ssh-keygen -y -f "${HOME}/.ssh/id_rsa" >> "${HOME}/.ssh/authorized_keys"
mkdir -p ${HOME}/.ssh
Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably not needed

@agoscinski
Copy link
Contributor Author

Will check if this work and remove the ssh part into another PR, because it is orthogonal to this one

The test compares the timezones between a fixed date and of the current date.
This causes problem with time zones that use daylight saving time (DST). Fixing
the test makes would add too much complexity for what the test is actually
covering.  The `make_aware` function is still thoroughly tested by the remaining
tests.
@agoscinski agoscinski force-pushed the devops/tox-presto branch 2 times, most recently from 88c0ca4 to 92b3cde Compare September 11, 2024 10:31


def test_aiida_config_tmp(aiida_config_tmp):
def test_aiida_config_tmp(aiida_config_tmp, tmp_path_factory):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needed to specify for pre-commit

pytest's tmp_path and tempfile returns a different directory on macOS Sonoma
than tempfile therefore we change the tests tmp_path as it is also used in in
the tests to create the config file.
agoscinski and others added 3 commits September 11, 2024 12:40
Before the path for `bash` and `cat` path was hardcoded. This has been changed
to run a subprocess running `which bash` and `which cat` to determine the path.
Co-authored-by: Sebastiaan Huber <[email protected]>
@agoscinski
Copy link
Contributor Author

I removed now the ssh part and will reintroduce this in another PR

@agoscinski
Copy link
Contributor Author

I would still keep the commit structure and merge them as Rebase and merge but the applied review commit has to be splitted up. I just did not want to do it, to simplify review. So if this is approved I can do it.

agoscinski added a commit to agoscinski/aiida-core that referenced this pull request Sep 11, 2024
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.

3 participants