Use pytest and temporary directories #54

TomNicholas · 2024-08-26T19:19:38Z

WIP to use pytest for the tests.

EDIT: I'm trying to change our tests from a run-once python script to an actual extensible testing framework. In particular that means tests should fully clean up after themselves automatically, and example blueprint generation should be extensible and robust.

TomNicholas · 2024-08-26T19:20:12Z

When I run pytest in the root I get this error:

E               OSError: System environment variable 'MARBL_ROOT' points toa github repository whose remote: 
E                '' 
E               does not match that expected by C-Star: 
E               https://github.com/marbl-ecosys/MARBL.git.Your environment may be misconfigured.

cstar/base/basemodel.py:221: OSError

dafyddstephenson · 2024-08-26T20:10:47Z

This is most likely because you deleted your externals and didn't delete cstar_local_config.py which tracked the (now non-existent) location of MARBL and ROMS on your system

TomNicholas · 2024-08-27T16:38:18Z

That gets me further, thanks!

NoraLoose · 2024-09-09T16:40:59Z

What's the status on this? Let me know if I can help @TomNicholas

cstar/tests/test_roms_marbl_example.py

TomNicholas · 2024-09-12T16:51:27Z

cstar/tests/test_roms_marbl_example.py

+@pytest.fixture
+def roms_marbl_blueprint_remote_datasets(tmp_path) -> Path:
+    """Creates blueprint yaml file in temporary directory"""
+
+    with open(ROMS_MARBL_BASE_BLUEPRINT_PATH, 'r') as file:
+        blueprint_dict = yaml.load(file, Loader=yaml.Loader)
+
+    blueprint_filepath = tmp_path / 'blueprint.yaml'
+
+    with open(blueprint_filepath, 'w') as file:
+        yaml.dump(blueprint_dict, file)
+
+    return blueprint_filepath
+
+
+# TODO similar fixture but the blueprint file contains paths to local files instead
+# TODO should these local files be in a given temporary directory?


I'm not entirely sure what the best way to handle blueprints is, but I think we should change the current approach of modifying a blueprint to use machine-specific local filepaths.

The point of this fixture was supposed to be so that we could start with one blueprint and modify it in-memory, then write the modified blueprint back out to a temporary directory, then pass the (temporary) path to that file back into the test.

However it would be better to just have a set of pre-defined blueprint files to read from. If paths in blueprints can be relative paths, then perhaps we can avoid having to make the blueprint specific to a machine?

However it would be better to just have a set of pre-defined blueprint files to read from.

I'm not strictly against having two separate blueprints (one local, one remote) if that's what you mean here. However, I deliberately avoided this as these two files would differ only by a single substring throughout. Any change to the remote blueprint thus needs to be also tracked in the local version (this could be a lot if we switch all the input datasets, for example) whereas currently we only need to track the two substrings (base URL, base filepath).

I'm not entirely sure what the best way to handle blueprints is, but I think we should change the current approach of modifying a blueprint to use machine-specific local filepaths.

This is definitely fine for testing, and any user is free to make blueprints with relative paths, but I would maintain the behaviour that, within C-Star, and in any blueprints it writes out, absolute paths are used (to maintain the most exact information available about the location of a file). C-Star-produced blueprints should not break when moved from one directory to another because of the use of relative paths. Thus if we want to use relative paths in a blueprint describing local files produced by C-Star, we should have to manually modify it.

I just stumbled on this discussion #46 which is similar. I forgot another reason for recycling the same blueprint was to test C-Star's ability to create a case from a C-Star-created blueprint. This is something we should definitely test if we aim to prepare separate local/remote blueprints. It may also be worth planning around that this will soon double to local/remote x netcdf/yaml ?

I'm not strictly against having two separate blueprints (one local, one remote) if that's what you mean here.

That was my second suggestion yes.

However, I deliberately avoided this as these two files would differ only by a single substring throughout. Any change to the remote blueprint thus needs to be also tracked in the local version (this could be a lot if we switch all the input datasets, for example)

Yeah, that's why I made this yaml fixture that reads the blueprint into memory. We could alter the yaml structure at this point, then parametrize that and auto-generate the different blueprints we need as we need them. The result would be similar to what you're already doing but hopefully less error-prone because the base blueprint is unchanging and the per-test blueprints are created (and deleted) as needed.

C-Star-produced blueprints should not break when moved from one directory to another because of the use of relative paths.

That's fair.

I forgot another reason for recycling the same blueprint was to test C-Star's ability to create a case from a C-Star-created blueprint.

We should test this, but in a dedicated test. I'm trying to think how to structure all this so that we don't just end up with a small number of mega-tests.

It may also be worth planning around that this will soon double to local/remote x netcdf/yaml ?

This is why I want to be able to test one thing per test with the blueprints etc. auto-generated as needed. Then we can just add more @pytest.mark.parametrize decorators to tests and we will cover all the possibilities.

Thanks for explaining, this makes a lot more sense to me now. Keeping a single yaml dict in memory then modifying it between tests is a very clean way to handle it, nice.

This generation and modification of the yaml as a dict in-memory is now implemented.

…for saving to a filepath

TomNicholas · 2024-09-13T20:32:26Z

cstar/tests/test_roms_marbl_example.py

+        # TODO define `make_local`function
+        if make_local:
+            # blueprint_dict = make_local(blueprint_dict)
+            raise NotImplementedError()


@dafyddstephenson do you think this approach will work? Is the difference between your two tests representable like this? I want the make_local function to make the same changes that you do here:

C-Star/tests/test_roms_marbl_example.py

Line 58 in c83b1a1

# Modify the blueprint file to point to local paths whenever we have the files:

Also how am I supposed to know what the value of these needs to be for any given test?

id_url_prefix = "https://github.com/CWorthy-ocean/input_datasets_roms_marbl_example/raw/main/" ac_url = "https://github.com/CWorthy-ocean/cstar_blueprint_roms_marbl_example.git"

I'm not really sure what's going on here I don't really see why not represent that change as a function, it's just a string replacement (url -> path).

they're in the blueprint, those are just the strings to replace. If you're only working with one core blueprint, then these won't change.

why not represent that change as a function

I was just wondering if that was sufficient.

If you're only working with one core blueprint, then these won't change.

Okay, sounds like I can hard code these values for now and revisit this in a follow-up to generalize it.

I've generalized these by putting them all in a top-level global EXAMPLE_BLUEPRINTS dict.

TomNicholas · 2024-09-15T02:10:10Z

The CI failure is #66 (separate from this PR).

TomNicholas · 2024-09-15T02:25:35Z

conftest.py

+    """Given the name of a pre-defined blueprint, returns it as a (temporary) path to an on-disk file."""
+
+    def _blueprint_as_path(name: str, make_local: bool = False) -> Path:
+        # TODO add options to edit in-memory blueprints here?


make_local is an example of one such option.

TomNicholas · 2024-09-15T02:28:05Z

cstar/tests/test_roms_marbl_example.py

+            roms_marbl_case.setup()
+
+            # TODO why are we persisting this blueprint file then not using it again in the test?
+            roms_marbl_case.persist(tmpdir / "test_blueprint.yaml")
+
+            roms_marbl_case.build()
+            roms_marbl_case.pre_run()
+            roms_marbl_case.run()
+            roms_marbl_case.post_run()


As an aside, I would like us to have tests for each of these steps individually.

TomNicholas · 2024-09-19T20:51:55Z

We decided that we really need a way to generate local test data on demand in order to perform tests with local data. Now that roms-tools is integrated with C-Star, we can do that using roms-tools. This could take the form of a monster fixture that creates input data ready for C-Star to use.

Given the size of that effort, it should happen in a different PR to this one. But we also don't want to leave this PR dangling. But if we merge it in we don't want to delete existing tests, as currently this PR has smaller coverage.

So @dafyddstephenson and I decided to merge basically what is here but without altering the existing tests. There will be an awkward crossover period with two independent sets of tests existing in main at once, but that seems like the least worst way to do it.

TomNicholas · 2024-09-20T21:15:56Z

.github/workflows/tests.yaml

+           coverage run --rcfile=coverage.toml -m pytest --verbose cstar/tests/*
           python -u -m coverage run --rcfile=coverage.toml tests/test_roms_marbl_example.py


This PR now runs both the old and the new tests.

Any chance we could separate these into two separate steps in the workflow? Hard to tell where one ends and the next begins

Fair point. I actually should probably just make them separate CI jobs.

TomNicholas · 2024-09-20T21:18:28Z

@dafyddstephenson I think this might be ready to merge now? It runs both sets of tests in the CI successfully, and you can run either one independently locally. #99 can be addressed in a follow-up PR.

dafyddstephenson · 2024-09-20T22:04:52Z

Sorry I should have taken a better look before your last commit - I think having tests and cstar/tests is a little confusing, perhaps renaming tests to tests_old in line with the CI job names or something. Other than that looks good to go.

TomNicholas · 2024-09-20T22:30:12Z

Moved to tests_old. The additional changes are just from pre-commit / to shut mypy up.

TomNicholas added 2 commits August 26, 2024 15:16

sketch out pytest version of roms_marbl test

611c8f3

add pytest to environment

31c2a44

TomNicholas added the testing label Aug 26, 2024

TomNicholas added 5 commits September 12, 2024 11:44

Merge branch 'main' into pytest

f72055a

add fixture for mocking user input

15d4549

moved roms_marbl test to tests subdirectory

aaeb6c4

add init

0b0c5d5

improve comment

72b6860

TomNicholas commented Sep 12, 2024

View reviewed changes

cstar/tests/test_roms_marbl_example.py Outdated Show resolved Hide resolved

TomNicholas commented Sep 12, 2024

View reviewed changes

TomNicholas changed the title ~~pytest~~ Use pytest and temporary directories Sep 12, 2024

TomNicholas added 9 commits September 12, 2024 15:40

linting

1887562

make pytest ignore externals so that we can delete cleanup steps

64233fe

clean up imports

b6a1f4d

link to issue

d425021

call pytest in the CI

9bad451

split blueprint fixture into one for creating in-memory dict and one …

6e310ed

…for saving to a filepath

refactors to use a global dictionary of paths to blueprint files

575ac3f

rename with example_ prefix

e4537bb

sketch of local parameter

cd2b6a4

TomNicholas commented Sep 13, 2024

View reviewed changes

TomNicholas added 3 commits September 14, 2024 01:54

surprisingly this passes

ba0960b

move fixtures to conftest.py

93d8548

parametrize the local and remote test into one

7366f6a

TomNicholas marked this pull request as ready for review September 14, 2024 06:14

TomNicholas commented Sep 15, 2024

View reviewed changes

TomNicholas mentioned this pull request Sep 15, 2024

Input datasets from roms tools #81

Merged

TomNicholas added 8 commits September 16, 2024 12:20

Merge branch 'main' into pytest

d4671de

Merge branch 'main' into pytest

e3a7acd

only mock user input where its actually needed

0cf32f9

move test blueprints list to tests directory

da6934b

move fixtures for creating blueprints into their own directory

c87d20b

test fixtures which manipulate yam templatel

25bff2b

move roms_marbl example test to dedicated directory

08a13c1

test treating blueprint_as_dict and blueprint_as_filepath the same

c84252b

disable testing with local data

c765f75

dafyddstephenson mentioned this pull request Sep 19, 2024

Create end-to-end minimal example for testing #99

Closed

5 tasks

TomNicholas added 4 commits September 19, 2024 17:34

also modify additional_code location

460fc0c

Merge branch 'main' into pytest

0134365

try to have pytest ignore re-included old tests

7683ef1

try running both old and new tests in same CI job

94728c9

TomNicholas commented Sep 20, 2024

View reviewed changes

Merge branch 'main' into pytest

ceb3388

separate tests into two CI jobs

e726366

TomNicholas added 3 commits September 20, 2024 18:13

don't run old tests in new tests CI job

1d86d57

move old tests to cstar/tests_old

82cccb2

satisfy mypy

b543aab

dafyddstephenson approved these changes Sep 20, 2024

View reviewed changes

dafyddstephenson mentioned this pull request Sep 20, 2024

Also run test_roms_tools_integration.py in CI #105

Closed

TomNicholas merged commit dc7d7db into CWorthy-ocean:main Sep 20, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pytest and temporary directories #54

Use pytest and temporary directories #54

TomNicholas commented Aug 26, 2024 •

edited

Loading

TomNicholas commented Aug 26, 2024

dafyddstephenson commented Aug 26, 2024

TomNicholas commented Aug 27, 2024

NoraLoose commented Sep 9, 2024

TomNicholas Sep 12, 2024

dafyddstephenson Sep 12, 2024

dafyddstephenson Sep 12, 2024 •

edited

Loading

TomNicholas Sep 12, 2024 •

edited

Loading

dafyddstephenson Sep 12, 2024

TomNicholas Sep 14, 2024

TomNicholas Sep 13, 2024 •

edited

Loading

TomNicholas Sep 14, 2024

dafyddstephenson Sep 14, 2024

dafyddstephenson Sep 14, 2024

TomNicholas Sep 14, 2024

TomNicholas Sep 14, 2024

TomNicholas commented Sep 15, 2024

TomNicholas Sep 15, 2024

TomNicholas Sep 15, 2024 •

edited

Loading

TomNicholas commented Sep 19, 2024

TomNicholas Sep 20, 2024

dafyddstephenson Sep 20, 2024

TomNicholas Sep 20, 2024

TomNicholas commented Sep 20, 2024

dafyddstephenson commented Sep 20, 2024

TomNicholas commented Sep 20, 2024

		coverage run --rcfile=coverage.toml -m pytest --verbose cstar/tests/*
		python -u -m coverage run --rcfile=coverage.toml tests/test_roms_marbl_example.py

Use pytest and temporary directories #54

Use pytest and temporary directories #54

Conversation

TomNicholas commented Aug 26, 2024 • edited Loading

TomNicholas commented Aug 26, 2024

dafyddstephenson commented Aug 26, 2024

TomNicholas commented Aug 27, 2024

NoraLoose commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dafyddstephenson Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

TomNicholas Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas Sep 13, 2024 • edited Loading

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

TomNicholas commented Sep 15, 2024

Choose a reason for hiding this comment

TomNicholas Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

TomNicholas commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomNicholas commented Sep 20, 2024

dafyddstephenson commented Sep 20, 2024

TomNicholas commented Sep 20, 2024

TomNicholas commented Aug 26, 2024 •

edited

Loading

dafyddstephenson Sep 12, 2024 •

edited

Loading

TomNicholas Sep 12, 2024 •

edited

Loading

TomNicholas Sep 13, 2024 •

edited

Loading

TomNicholas Sep 15, 2024 •

edited

Loading