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

Null tests.142 #290

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

Null tests.142 #290

wants to merge 45 commits into from

Conversation

kiihne-noaa
Copy link
Contributor

Describe your changes

Added tests in fre/make/null_example for checkout

Issue ticket number and link (if applicable)

creating this so Dana can help with debugging of issues with paths

Checklist before requesting a review

@singhd789

@singhd789 singhd789 self-requested a review December 11, 2024 16:41
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/test_basic_null_yaml.py Outdated Show resolved Hide resolved
@ilaflott
Copy link
Member

for the rm -rf, instead of subprocess, consider shutil.rmtree

"""
check if --verbose option works
"""
create_checkout_script.checkout_create(YAMLFILE,PLATFORM,TARGET,False,False, False,True)
Copy link
Member

Choose a reason for hiding this comment

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

@singhd789 @thomas-robinson perhaps create_checkout_script should return the full path to checkout.sh? instead of just printing it to screen?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, the whole path to the checkout script is printed (and it's made executable) - that way the user knows where it is and could also run it themselves if they want. What would be another benefit of returning it as well? Just curious

Copy link
Member

Choose a reason for hiding this comment

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

this test + the otheres could become a simple:

compile_sh_path =  create_checkout_script (<BLAH ARGS>)
assert Path(compile_sh_path).exists()

similarly, elsewhere in the code base, if create_checkout_script is being used, that returned string object could be actually leveraged by other functions. If it's just print, then there is no option to do so

check if --execute option works
"""
subprocess.run(["rm","-rf",f"{OUT}/fremake_canopy/test"])
create_checkout_script.checkout_create(YAMLFILE,PLATFORM,TARGET,False,2, True,False)
Copy link
Member

Choose a reason for hiding this comment

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

OK so i saw that what brought this PR's checks from red-x to green-check is the change of this functions argument from a boolean value to an integer 2. This suggests to me that it's hard to track what these arguments are supposed to be when developing the code, so checkout_create needs explicit keyword arguments, not positional.

this line should read like:

create_checkout_script.checkout_create(
    yamlfile = YAMLFILE, 
    platform = PLATFORM, 
    target = TARGET, 
    no_parallel_checkout = False, 
    jobs = 2, 
    execute = True, 
    verbose = False )

fre/make/tests/test_create_checkout.py Show resolved Hide resolved
fre/make/tests/null_example/test_checkout_null_yaml.py Outdated Show resolved Hide resolved
fre/make/tests/test_create_checkout.py Show resolved Hide resolved
target = "prod-openmp"

# Set example yaml paths, input directory
CWD = Path.cwd()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you use the ci.gnu platform with the TEST_BUILD_DIR defined for the modelRoot (

modelRoot: ${TEST_BUILD_DIR}/fremake_canopy/test
), I don't think you'll even need CWD

@@ -18,7 +18,9 @@ def checkout_create(yamlfile, platform, target, no_parallel_checkout, jobs, exec
run = execute
jobs = str(jobs)
pcheck = no_parallel_checkout


if jobs == 'False' and execute:
Copy link
Collaborator

@singhd789 singhd789 Jan 27, 2025

Choose a reason for hiding this comment

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

I think jobs has a default value here (Default value set in fremake.py click options for jobs - when you don't explicitly define it, 4 prints out). So it might always be "True" or defined.

@kiihne-noaa
Copy link
Contributor Author

@singhd789 @ilaflott This is now ready for final review

merging main in, before doing the reverse
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