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

Fixing Postgres tests #2250

Draft
wants to merge 2 commits into
base: as-postgres
Choose a base branch
from
Draft

Fixing Postgres tests #2250

wants to merge 2 commits into from

Conversation

kinow
Copy link
Member

@kinow kinow commented Mar 26, 2025

Will try to continue tomorrow and finish before the end of the day on Friday.

  • Fix integration test run command test (failing intermittently locally)
  • Remove some mock from autosubmit_config, test with a real experiment
  • Move database tests to database package (in tests)
  • Move tests that use Postgres & SQlite to the integration folder
  • Remove GH Actions Postgres, start with testcontainers context manager, so we can run it automatically in our laptops (like the SSH integration test)

@kinow kinow self-assigned this Mar 26, 2025
@kinow kinow added this to the 4.2.0 milestone Mar 26, 2025
@kinow kinow mentioned this pull request Mar 26, 2025
2 tasks
@kinow kinow changed the title Fixing WIP Fixing Postgres tests Mar 27, 2025
@kinow
Copy link
Member Author

kinow commented Mar 28, 2025

In case I don't finish this PR today, @LuiggiTenorioK , this PR includes work to fix test_run_command_integration which started to fail intermittently on my laptop. But we could try to fix the two tests in the other branch (because the work here includes invalid Python code -- I was really in the middle of writing the code).

But the idea for the tests, would be,

  • add postgres mark in pytest.ini and tests
  • use testcontainers doing something similar to
# integration/database/conftest.py
@pytest.fixture(scope='function', autouse=True)
def as_pg_fixture...():
    with PostgresContainer('postgres:16', port=??, username: 'autosubmit', ...) as postgres:
        psql_url = postgres.get_connection_url()
        # create AS objects, or update the connection setting, etc... using data for container above if needed
        yield postgres

# integration/database/some_test.py
def test_something():
    # every function in every test under integration/database/<FILE> should
    # have a database created -- if needed, we can set the scope to module, but
    # I think if we don't have hundreds of tests it should be executed in < 3 mins?
    ...

Then, a developer can run this locally with pytest -m "postgres", without having to start any container.

But in reality, developers will run pytest, which won't include the postgres tests. Same with pytest test/integration, it will not include postgres nor docker. This means AS devs can select if they want unit, integration, or integration + docker, or integration + postgres, or integration + postgres/docker, etc.

@LuiggiTenorioK
Copy link
Member

@kinow this looks quite interesting! Hope you make this work before leaving on holidays 🤞 Anyway I could try to fix it later.

The only thing that makes me worry is the high number of changes, which I hope doesn't make the main PostgreSQL branch hard to review. However, I could split the PR into 2 in the future to make it easier to review, if needed.

@kinow
Copy link
Member Author

kinow commented Mar 28, 2025

You are correct, @LuiggiTenorioK . It'd probably be easier for me to rebase this against master, remove the postgres changes, and get this merged separately (as this is not directly related to PG). But I will have to fix this once I'm back (definitely no time to finish that today).

FTR, here's an example we have in the code, of what I showed above, although that is not a fixture:

with DockerContainer(image=image, remove=True, hostname='openssh-server') \
.with_env('TZ', 'Etc/UTC') \
.with_env('SUDO_ACCESS', 'false') \
.with_env('USER_NAME', user) \
.with_env('USER_PASSWORD', 'password') \
.with_env('PASSWORD_ACCESS', 'true') \
.with_bind_ports(2222, ssh_port) \
.with_volume_mapping('/tmp', '/tmp', mode='rw') as container:
wait_for_logs(container, 'sshd is listening on port 2222')
_ssh = paramiko.SSHClient()
_ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
_ssh.connect(hostname=platform.host, username=platform.user, password='password', port=ssh_port)
platform._ftpChannel = paramiko.SFTPClient.from_transport(_ssh.get_transport(), window_size=pow(4, 12),
max_packet_size=pow(4, 12))
platform._ftpChannel.get_channel().settimeout(120)
platform.connected = True
platform.get_send_file_cmd = mocker.Mock()
platform.get_send_file_cmd.return_value = 'ls'
platform.send_command = mocker.Mock()
platform.send_file(filename)
assert check == (remote_dir / filename).exists()
finally:
os.system(f'chmod 700 -R {str(rootdir)}')

👋

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.

2 participants