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

add fre make create-dockerfile test #328

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rem1776
Copy link
Contributor

@rem1776 rem1776 commented Jan 22, 2025

Describe your changes

Adds a test file for create-dockerfile with any tests that don't involve building the image (those will come alongside some more run-fremake tests once I can get a small enough base image to fit).

One issue I had was the dockerfile command would fail if create-checkout wasn't called before it, so this PR also adds a line that will create the tmp directory if it needs to. Now you can technically run the create-dockerfile command before the checkout script and makefile are created. I'm not sure if thats a good thing so lmk if this should work differently.

Issue ticket number and link (if applicable)

#225

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback

fre/make/tests/test_create_dockerfile.py Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another good test to add would be to check if a linkline.sh script is created if container_addlibs are defined as well (the bit that happens in this if statement in makefilefre.py:

if "tmp" in self.filePath:
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried adding this in, but it seems like the makefile creation function isn't run by create-dockerfile. Maybe I should add that to the create-makefile test with a different PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh yes yes, you're right. That script is created in the makefile step, then copied into the dockerfile in this tool. I agree, the creation of this file would be in the makefile test.

Hmm, if we did want to somehow eventually add a test for a successful dockerfile execute with container_addlibs defined, we'd probably need an example linkline script (that would work for whatever additional libraries we add in there), or something. Maybe a future test to think about 🤔

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