WP5 indiv process testing: code style improvements #21

soxofaan · 2024-01-16T11:06:10Z

@m-mohr I went through .../processing/test_example.py while playing with the individual process tests and made some minor code style tweaks along the way.
I wanted to run them with you first before merging into main.

included tweaks:

avoid in-place dict mutations
add type hints
use more keyword args for call robustness
more clearly mark internal helpers as private
finetune exception handling
finetune pytest asserts (where default value rendering should be good enough)

- avoid in-place dict mutations - add type hints - use more keyword args for call robustness - more clearly mark internal helpers as private - finetune exception handling - finetune pytest asserts (where default value rendering should be good enough)

src/openeo_test_suite/tests/processes/processing/test_example.py

m-mohr · 2024-01-16T11:58:32Z

src/openeo_test_suite/tests/processes/processing/test_example.py

    except Exception as e:
-        # TODO: skipping on a generic `Exception` is very liberal and might hide real issues


Unless we have a more specific handler, this should be kept for now and not just raise the exceptions. It's a behavioral change, not a code style change.

This liberal except clause hid a bug I introduced with this PR.
Luckily I noticed it in another way.

I guess your intention is to skip when something is not implemented, so I would propose to change it to
except NotImplementedError

I can only check what I meant once I can run it again ;-)

I'm going through it now to check what changes if we change to NotImplementedError and if the additional failures are test issues etc. Stay tuned ;-)

I just pushed 7507407 which switches to only NotImplementedError and fixes a couple of mostly datetime-related things in the background so that test coverage is still similar. Feel free to have a look at the changes and whether they are inline with your changes.

src/openeo_test_suite/tests/processes/processing/test_example.py

m-mohr

Thanks. Some change requests, otherwise looks good in theory.
Wasn't able to run it, it looks like a recent merge corrupted the individual process tests.
pytest src/openeo_test_suite/tests/processes/processing/test_example.py --runner=dask worked before, is not running any longer.

soxofaan · 2024-01-16T13:18:41Z

Wasn't able to run it, it looks like a recent merge corrupted the individual process tests.
pytest src/openeo_test_suite/tests/processes/processing/test_example.py --runner=dask worked before, is not running any longer.

what error do you get? Please create a bug ticket if that makes sense

m-mohr · 2024-01-16T13:27:48Z

Done: #23

soxofaan · 2024-01-16T18:34:16Z

I restored the liberal except: pytest.skip() logic and moved the custom assert messages back.
Unless you have more remarks, I'd like to move this forward and merge it.
(I'd like to work on some more unification across the different WPs and that would conflict too easily with this PR I guess)

m-mohr · 2024-01-17T09:15:35Z

src/openeo_test_suite/tests/processes/processing/test_example.py

-                    example["throws"], result.__class__.__name__
-                )
+            # TODO: better way to report this warning?
+            _log.warning(


I don't get this warning anylonger in the logs, I tried before this PR and after and these ones are missing. Any clue why?

logs of successful tests are hidden by default.
logs of failing tests should be visible

you can disable capturing of all logs (and print statements) with options --capture=no --log-cli-level=INFO

But that shouldn't have changed. The number of successful/failing tests is the same, just the logs for this specific line are missing.

do you have an example use case (process that fails on some implementation) for this, so I can play with this too?

Run pytest src/openeo_test_suite/tests/processes/processing/test_example.py --runner=dask both on this branch and on master.

You can then see differences:

soxofaan · 2024-01-19T20:58:06Z

hmm this PR got in conflicting state with main branch.
I think I'm going to abort this, and just cherry pick some minor things

- avoid in-place dict mutations - add type hints - use more keyword args for call robustness - more clearly mark internal helpers as private - finetune exception handling - finetune pytest asserts (where default value rendering should be good enough) Rebased version of PR #21

soxofaan · 2024-01-19T21:45:34Z

I see you merged main in this PR, but at the same time I managed to rebase PR #21 on latest main branch, and created new PR #32, which will give a cleaner history

I'm also considering to just cherrypick you "fix temporal issues" commit on util.py, to get that out of the way already

m-mohr · 2024-01-19T22:03:08Z

See #32

- avoid in-place dict mutations - add type hints - use more keyword args for call robustness - more clearly mark internal helpers as private - finetune exception handling - finetune pytest asserts (where default value rendering should be good enough) Rebased version of PR #21

soxofaan requested a review from m-mohr January 16, 2024 11:06

m-mohr reviewed Jan 16, 2024

View reviewed changes

src/openeo_test_suite/tests/processes/processing/test_example.py Outdated Show resolved Hide resolved

m-mohr reviewed Jan 16, 2024

View reviewed changes

src/openeo_test_suite/tests/processes/processing/test_example.py Outdated Show resolved Hide resolved

m-mohr reviewed Jan 16, 2024

View reviewed changes

src/openeo_test_suite/tests/processes/processing/test_example.py Outdated Show resolved Hide resolved

m-mohr reviewed Jan 16, 2024

View reviewed changes

src/openeo_test_suite/tests/processes/processing/test_example.py Outdated Show resolved Hide resolved

m-mohr requested changes Jan 16, 2024

View reviewed changes

soxofaan added 3 commits January 16, 2024 18:53

Merge branch 'main' into indiv-process-tests-code-style-tweaks

d86d93b

PR #21 restore some original logic and assert messages

b0558a5

Improve math.isnan check

5779218

soxofaan requested a review from m-mohr January 16, 2024 18:30

Merge branch 'main' into indiv-process-tests-code-style-tweaks

fe7224e

m-mohr reviewed Jan 17, 2024

View reviewed changes

Fix temporal issues and some other improvements

7507407

m-mohr force-pushed the indiv-process-tests-code-style-tweaks branch from 7b28019 to 7507407 Compare January 17, 2024 13:20

soxofaan marked this pull request as draft January 19, 2024 20:57

soxofaan added a commit that referenced this pull request Jan 19, 2024

Update assets/processes submodule (PR #21 related)

6d62ebc

soxofaan mentioned this pull request Jan 19, 2024

WP5 indiv process testing: code style improvements #32

Closed

Merge branch 'main' into indiv-process-tests-code-style-tweaks

3742813

m-mohr closed this Jan 19, 2024

m-mohr deleted the indiv-process-tests-code-style-tweaks branch January 19, 2024 22:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WP5 indiv process testing: code style improvements #21

WP5 indiv process testing: code style improvements #21

soxofaan commented Jan 16, 2024

m-mohr Jan 16, 2024 •

edited

Loading

soxofaan Jan 16, 2024

m-mohr Jan 16, 2024

m-mohr Jan 17, 2024

m-mohr Jan 17, 2024 •

edited

Loading

m-mohr left a comment •

edited

Loading

soxofaan commented Jan 16, 2024

m-mohr commented Jan 16, 2024

soxofaan commented Jan 16, 2024

m-mohr Jan 17, 2024

soxofaan Jan 17, 2024

m-mohr Jan 17, 2024 •

edited

Loading

soxofaan Jan 17, 2024

m-mohr Jan 17, 2024

soxofaan commented Jan 19, 2024

soxofaan commented Jan 19, 2024

m-mohr commented Jan 19, 2024

		except Exception as e:
		# TODO: skipping on a generic `Exception` is very liberal and might hide real issues

WP5 indiv process testing: code style improvements #21

WP5 indiv process testing: code style improvements #21

Conversation

soxofaan commented Jan 16, 2024

m-mohr Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

soxofaan Jan 16, 2024

Choose a reason for hiding this comment

m-mohr Jan 16, 2024

Choose a reason for hiding this comment

m-mohr Jan 17, 2024

Choose a reason for hiding this comment

m-mohr Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

m-mohr left a comment • edited Loading

Choose a reason for hiding this comment

soxofaan commented Jan 16, 2024

m-mohr commented Jan 16, 2024

soxofaan commented Jan 16, 2024

m-mohr Jan 17, 2024

Choose a reason for hiding this comment

soxofaan Jan 17, 2024

Choose a reason for hiding this comment

m-mohr Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

soxofaan Jan 17, 2024

Choose a reason for hiding this comment

m-mohr Jan 17, 2024

Choose a reason for hiding this comment

soxofaan commented Jan 19, 2024

soxofaan commented Jan 19, 2024

m-mohr commented Jan 19, 2024

m-mohr Jan 16, 2024 •

edited

Loading

m-mohr Jan 17, 2024 •

edited

Loading

m-mohr left a comment •

edited

Loading

m-mohr Jan 17, 2024 •

edited

Loading