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

Base test class #233

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

diegolovison
Copy link
Contributor

This PR starts by renaming SshTestBase to SshContainerTestBase in order to keep the current tests in the same state. Then, we create the SshTestBase by adding the generic methods.
This class allows the developers to easily test scripts locally ( they understand the side effects that will change the local environment ).
Our long-term goal is to support Podman and Docker but we are still not there and having SshTestBase allows the developer to have something ready to use.

I will give you a concrete example:
The following command failed and the error message is clear. We should use something like user@hostName

$ qdup hyperfoil.yaml -ix -S target-host='$($USER:localhost)'
Error: failed to create host from ${{target-host}}
  role:  stage: pending script: 
  command: 
Error: Role setup Host test was added without a fully qualified host representation matching user@hostName:port
 hosts:{test=${{target-host}}}
  role:  stage: pending script: 
  command: 
Error: Role hyperfoil Host test was added without a fully qualified host representation matching user@hostName:port
 hosts:{test=${{target-host}}}
  role:  stage: pending script: 
  command: 
Error: missing host for test
  role:  stage: pending script: 
  command: 
Error: missing host for test
  role:  stage: pending script: 
  command: 

As a developer, I would like to debug and learn the code base.
With this PR, the developer can do the following

public class MyTest extends SshTestBase {

    @Test
    public void wrongHost() {
        Parser parser = Parser.getInstance();
        RunConfigBuilder builder = getBuilder();
        builder.loadYaml(parser.loadFile("example",stream("""
            scripts:
              hello-qdup:
                - sh: echo hello-qdup
            hosts:
              local : ${{target-host}}
            roles:
              run-hello-qdup:
                hosts:
                  - local
                run-scripts:
                  - hello-qdup
        """)));
        builder.forceRunState("target-host", System.getenv("USER") + ":localhost");
        RunConfig config = builder.buildConfig(parser);
        assertFalse("runConfig errors:\n" + config.getErrorStrings().stream().collect(Collectors.joining("\n")), config.hasErrors());

        Dispatcher dispatcher = new Dispatcher();
        Run doit = new Run(tmpDir.toString(), config, dispatcher);
        doit.run();

        State state = config.getState();
    }
}

@diegolovison diegolovison changed the title Base test Base test class Jan 14, 2025
@willr3
Copy link
Collaborator

willr3 commented Jan 15, 2025

I do not think we need to refactor SshTestBase to allow developers to create tests that target an ssh service running on their machines instead of an ssh service in a container.

The example code above relies on three parts of SshTestBase: stream, getBuilder, and tmpDir.

The stream method is leftover from before java added multi-line string literals an can be removed because your example is using a multi-line string literal.

The getBuilder() method provides a builder with a custom identity file and ensures the folders containing the identify file have the correct permission. This could be replaced in your example with:

RunConfigBuilder builder = new RunConfigBuilder();

assuming the developer uses the default identity file (~/.ssh/id_rsa) otherwise they will need to set the identity

RunConfigBuilder builder = new RunConfigBuilder();
builder.setIdentity("/tmp/myIdentity");

The tmpDir was probably added to keep tack of temporary folders during testing but looking at that part of the code I think it could use a refactor.

The example MyTest is possible without the refactored SshTestBase as it does not need a base class

package io.hyperfoil.tools.qdup;

import io.hyperfoil.tools.qdup.cmd.Dispatcher;
import io.hyperfoil.tools.qdup.config.RunConfig;
import io.hyperfoil.tools.qdup.config.RunConfigBuilder;
import io.hyperfoil.tools.qdup.config.yaml.Parser;
import org.junit.Test;

import java.util.stream.Collectors;

import static org.junit.Assert.assertFalse;

public class MyTest {
    @Test
    public void wrongHost() {
        Parser parser = Parser.getInstance();
        RunConfigBuilder builder = new RunConfigBuilder();
        builder.loadYaml(parser.loadFile("example",stream("""
            scripts:
              hello-qdup:
                - sh: echo hello-qdup
            hosts:
              local : ${{target-host}}
            roles:
              run-hello-qdup:
                hosts:
                  - local
                run-scripts:
                  - hello-qdup
        """)));
        builder.forceRunState("target-host", System.getenv("USER") + ":localhost");
        RunConfig config = builder.buildConfig(parser);
        assertFalse("runConfig errors:\n" + config.getErrorStrings().stream().collect(Collectors.joining("\n")), config.hasErrors());

        Dispatcher dispatcher = new Dispatcher();
        Run doit = new Run(tmpDir.toString(), config, dispatcher);
        doit.run();

        State state = config.getState();
    }
}

I replaced getBuilder() with new RunConfigBuilder(), removed stream and replaced tmpDir with "/tmp"

Part of the reason we moved to using TestContainers as the target for qDup testing is to avoid polluting the local developer environment with the system mutations that occur during testing. We also do not want developers to have to add specific identity files to their authorized_keys as that would be a significant security problem.

It would be great if we could change the test suite to run against a variety of ssh targets. For example, we have run into issues in the past where the target ssh server is running a different shell. It would help if we could test against a variety of target environments via TestContainers.

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