Null tests.142 #290

kiihne-noaa · 2024-12-11T16:40:29Z

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 · 2024-12-11T17:03:48Z

fre/make/tests/null_example/test_checkout_null_yaml.py

@@ -0,0 +1,45 @@
+#tests for the create-checkout step of fre-make, for null_model.yaml
+import os
+from fre import fre


Can remove this, not needed

singhd789 · 2024-12-11T17:04:39Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+from fre.pp import configure_script_yaml as csy
+from click.testing import CliRunner


Can remove these too, not needed. We won't be using runner.invoke for these tool tests here

singhd789 · 2024-12-11T17:05:24Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+from fre.pp import configure_script_yaml as csy
+from click.testing import CliRunner
+import subprocess
+from fre.make import createCheckout


This needs to be updated: createCheckout --> create_checkout_script

singhd789 · 2024-12-11T17:07:37Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+test_dir = Path("fre/make/tests/null_example")
+yamlfile = Path(f"{test_dir}/null_model.yaml")
+
+#set platform and target
+platform = "ncrc5.intel"
+target = "debug"


To keep consistency with other tests, make these variable names capitalized.

singhd789 · 2024-12-11T17:08:45Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+test_dir = Path("fre/make/tests/null_example")
+yamlfile = Path(f"{test_dir}/null_model.yaml")


I'd recommend keeping all the tests together in fre/make/tests for now. We can move it there and make this

TEST_DIR = Path("fre/make/tests")
YAMLFILE = Path(f"{TEST_DIR}/null_example/null_model.yaml")

singhd789 · 2024-12-11T17:13:00Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+
+# Set home for ~/cylc-src location in script
+#os.environ["HOME"]=str(Path(f"{out_dir}"))
+HOME_DIR = os.environ["HOME"]


So through doing this, we've found it definitely effects other tests that use HOME.

Let's move re-setting HOME to inside each test that needs it AND make sure to save the default HOME location first. We can look at how Ryan maneuvered it:
define home:

fre-cli/fre/make/tests/test_create_makefile.py

Line 54 in 3c9cacb

def_home = str(os.environ["HOME"])

set home:

fre-cli/fre/make/tests/test_create_makefile.py

Line 55 in 3c9cacb

os.environ["HOME"]=OUT#str(Path(OUT))

reset home:

fre-cli/fre/make/tests/test_create_makefile.py

Line 64 in 3c9cacb

os.environ["HOME"] = def_home

singhd789 · 2024-12-11T17:15:22Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+#set output directory
+#out_dir = Path(f"fre/make/tests/null_example/fre_make_out")
+#Path(out_dir).mkdir(parents=True,exist_ok=True)


In my opinion, I feel like it would be nice it each test had output that was easy to find. Here, for the checkout, maybe we can set OUT to be:

OUT = f"{TEST_DIR}/checkout_out"

Where test_dir would be what I commented above. Also add in the out location to the .gitignore file

singhd789 · 2024-12-11T17:15:34Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+#os.environ["HOME"]=str(Path(f"{out_dir}"))
+HOME_DIR = os.environ["HOME"]
+#run checkout command
+runner = CliRunner()


singhd789 · 2024-12-11T17:20:25Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+    """
+    Make sure checkout file exists
+    """
+    result = runner.invoke(fre.fre, args=["make","create-checkout","-y",yamlfile,"-p",platform,"-t",target])


Using runner.invoke is good for testing the cli for fre. For actually script functionality testing here, we're going to call the actual functions in the script.

Using from fre.make import create_checkout_script from the top:

we'll set/reset home as I mentioned above

we'll set what would be click options passed by the user for the fre make create-checkout command

we'll invoke the main checkout function

check for the existence of the checkout script as a good starting test

You have line 33 that just needs to be edited a little. Try this:
create_checkout_script.checkout_create(variables set right before)

singhd789 · 2024-12-11T17:21:11Z

fre/make/tests/test_basic_null_yaml.py

Is this different than the test_basic_null_yaml.py file in fre/make/tests/null_example?

update local branch for change in fre make command names

singhd789 · 2025-01-13T20:42:16Z

fre/make/tests/test_basic_null_yaml.py

+test_yaml = Path(f"null_example/null_model.yaml")
+
+# Set home for ~/cylc-src location in script
+os.environ["HOME"]=str(Path(f"{CWD}/{test_dir}/configure_yaml_out"))


this is probably why we keep seeing some failures

singhd789 · 2025-01-13T20:43:14Z

fre/make/tests/test_basic_null_yaml.py

Is there a difference between this file and fre/make/tests/null_example/test_basic_null_yaml.py? I think we can get rid of the one in null_example/

singhd789 · 2025-01-13T20:45:30Z

fre/make/tests/null_example/test_checkout_null_yaml.py

2 things:

Can we put this with the other tests too? (Just in fre/make/tests - back one directory)

Can we rename this test_create_checkout.py? I think the trend is to name the test file after the tool it's testing.

ilaflott · 2025-01-22T18:02:41Z

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

ilaflott · 2025-01-22T18:08:02Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+    """
+    Make sure checkout file exists
+    """
+    subprocess.run(["rm","-rf",f"{OUT}/fremake_canopy/test/null_model_full/src/checkout.sh"])


the fremake_canopy/ part seems old/out-of-date. double check the directory structure being created locally and adjust what path you're attempting to remove and then check for in the assert below

Hm, yeah the fremake_canopy bit is actually a part of a path set in the platforms.yaml. Good point, it should be updated, but I don't thiiiiink that specific part is the problem. I still agree, always double check the path/directory structure for sure though!

ilaflott · 2025-01-22T18:09:04Z

fre/make/tests/null_example/test_checkout_null_yaml.py

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


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

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

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

ilaflott · 2025-01-27T15:30:41Z

fre/make/create_checkout_script.py

-
+
+    if jobs == 'False' and execute == True:
+        sys.exit('jobs must be defined as number if --execute flag is True')


don't use sys.exit, raise ValueError('text-here') or smth.

ilaflott · 2025-01-27T15:31:36Z

fre/make/create_checkout_script.py

@@ -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 == True:


why is jobs a string of a bool? Why are we checking if execute is True using an equality operator? Why can't this be- if not jobs and execute: ?

ilaflott · 2025-01-27T15:39:39Z

fre/make/tests/null_example/test_checkout_null_yaml.py

+    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)


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 )

ilaflott · 2025-01-27T15:40:52Z

fre/make/tests/test_create_checkout.py

+test_yaml = Path(f"null_example/null_model.yaml")
+
+# Set home for ~/cylc-src location in script
+os.environ["HOME"]=str(Path(f"{CWD}/{test_dir}/configure_yaml_out"))


str(Path(f"STUFF")) here could just be f"STUFF"

also os.environ["HOME"] needs to be set back to it's original value, or it will be set to f"{CWD}/{test_dir}/configure_yaml_out" for ALL other testing routine calls

Avery Kiihne and others added 14 commits October 9, 2024 15:42

#142 add basic tests for null

0e13bee

rearranged tests and added checkout test

413f922

testing if runner fixes bug # 142

c7a06ac

unfinished update to test path #222

2e2dcc3

test partly working #222

0ab4be6

Merge branch 'main' into null_tests.142

3b78d84

change to yaml path #222

2fb2724

improvements to syntax #222

2c0846f

functional checkout tests #222

34eb37e

adjusted path #222

ba1b40b

Merge branch 'main' into null_tests.142

cfda9af

testing the test

c295c8b

Update test_checkout_null_yaml.py

0d28382

Update test_checkout_null_yaml.py

2c41baf

singhd789 self-requested a review December 11, 2024 16:41

singhd789 requested changes Dec 11, 2024

View reviewed changes

error review

3f04df9

kiihne-noaa force-pushed the null_tests.142 branch from 2078d23 to 3f04df9 Compare December 16, 2024 20:46

kiihne-noaa and others added 7 commits December 16, 2024 15:53

adjust imports

70fb326

Merge pull request #296 from NOAA-GFDL/main

18def12

update local branch for change in fre make command names

Update test_checkout_null_yaml.py

4f51b8b

trying str paths

77f1bac

tests semi working

f8f3c7b

update checkout script print statements

fdf6bff

Merge branch 'main' into null_tests.142

c7c62d7

kiihne-noaa force-pushed the null_tests.142 branch from 7b22d9c to c7c62d7 Compare January 8, 2025 18:19

checkout test working locally

cd651df

singhd789 reviewed Jan 13, 2025

View reviewed changes

experimental changes

fe3b347

kiihne-noaa force-pushed the null_tests.142 branch from 7859ff6 to fe3b347 Compare January 14, 2025 16:27

kiihne-noaa added 11 commits January 14, 2025 11:29

Rename test_basic_null_yaml.py to test_create_checkout.py

69896af

Update test_checkout_null_yaml.py

da09b07

Update test_checkout_null_yaml.py

f1824e8

add verbose check

33c1831

remove rm statement

1dada88

Update test_checkout_null_yaml.py

91b82b0

fixing execute

1c3464c

Merge branch 'main' into null_tests.142

ba54159

testing

03bc67a

remove rm statement to check error message

24d021b

Update test_checkout_null_yaml.py

baa9293

ilaflott reviewed Jan 22, 2025

View reviewed changes

kiihne-noaa added 3 commits January 27, 2025 09:29

running execute with jobs num designated

320e270

added no parallel checkout test

3fcd58c

Update create_checkout_script.py to include new error message

848ecb2

ilaflott reviewed Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Null tests.142 #290

Null tests.142 #290

kiihne-noaa commented Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Dec 11, 2024

singhd789 Jan 13, 2025

singhd789 Jan 13, 2025

singhd789 Jan 13, 2025

ilaflott Jan 27, 2025

ilaflott commented Jan 22, 2025

ilaflott Jan 22, 2025

singhd789 Jan 22, 2025

ilaflott Jan 22, 2025

singhd789 Jan 22, 2025

ilaflott Jan 27, 2025

ilaflott Jan 27, 2025

ilaflott Jan 27, 2025

ilaflott Jan 27, 2025

ilaflott Jan 27, 2025

ilaflott Jan 27, 2025

		from fre.pp import configure_script_yaml as csy
		from click.testing import CliRunner

		test_dir = Path("fre/make/tests/null_example")
		yamlfile = Path(f"{test_dir}/null_model.yaml")



		if jobs == 'False' and execute == True:
		sys.exit('jobs must be defined as number if --execute flag is True')

Null tests.142 #290

Are you sure you want to change the base?

Null tests.142 #290

Conversation

kiihne-noaa commented Dec 11, 2024

Describe your changes

Issue ticket number and link (if applicable)

Checklist before requesting a review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilaflott commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment