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

Update CI/tests to allow running proper, realistic unit tests #136

Merged
merged 16 commits into from
Feb 17, 2022
Merged

Conversation

andersy005
Copy link
Member

@andersy005 andersy005 commented Feb 16, 2022

This PR is one step towards a more robust CI system. I modified a few methods but kept the functionality the same. I intend to look into a refactor of the RemoteRunner so as to facilitate testing it in a separate PR.

@andersy005 andersy005 changed the title Update CI to allow initiating SSH connection to localhost Update CI/tests to allow running proper, realistic unit tests Feb 17, 2022
@andersy005 andersy005 marked this pull request as ready for review February 17, 2022 04:51
@andersy005 andersy005 added enhancement New feature or request CI labels Feb 17, 2022
Copy link
Contributor

@mnlevy1981 mnlevy1981 left a comment

Choose a reason for hiding this comment

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

The code you've included looks good, but I think we should modify .github/workflows/ci.yaml to test with sh and tcsh in addition to bash. I think, as written, we should get passes from sh but test_run_command will fail on tcsh. I'm okay with accepting this PR with a failing test in that case, I just want the test in place before we start refactoring to make it easier to support multiple shell options.

@andersy005
Copy link
Member Author

I think we should modify .github/workflows/ci.yaml to test with sh and tcsh in addition to bash

I intend to address this within pytest unit tests instead of controlling this parametrization at the CI level. I avoided doing this in this PR since it would have required re-writing some parts of the code base and I wanted to make sure the SSH setup is working before breaking the tests...

Copy link
Contributor

@mnlevy1981 mnlevy1981 left a comment

Choose a reason for hiding this comment

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

Makes sense! Sorry if we talked about this earlier and I just misunderstood what the intended plan was :)

@andersy005
Copy link
Member Author

No worries... You are right. During our last conversation, we had talked about parameterizing shells at the CI level. However, I think parametrizing this in the unit tests gives us more flexibility/control.

@andersy005
Copy link
Member Author

Thank you for the feedback! I'm going to merge this and then I will open a separate PR that addresses #135

@andersy005 andersy005 merged commit 917c142 into main Feb 17, 2022
@andersy005 andersy005 deleted the ci branch February 17, 2022 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants