Change default PR-triggered test behavior

[leewujung]

Right now the PR-triggered tests are determined by the existence of the string [all tests ci] in the PR title. If this doesn't exist, only tests associated the module that was modified are run. This has caused quite a bit of problems since some code changes would break downstream cascading processing but would not be caught in this way.

echopype/.github/workflows/pr.yaml

Lines 72 to 79 in 3df9d83

	- name: Running all Tests
	if: contains(github.event.pull_request.title, '[all tests ci]')
	run: |
	pytest -vvv -rx --numprocesses=${{ env.NUM_WORKERS }} --max-worker-restart=3 --cov=echopype --cov-report=xml --log-cli-level=WARNING --disable-warnings
	- name: Running Tests
	if: ${{ !contains(github.event.pull_request.title, '[all tests ci]') }}
	run: |
	python .ci_helpers/run-test.py --pytest-args="--log-cli-level=WARNING,-vvv,-rx,--numprocesses=${{ env.NUM_WORKERS }},--max-worker-restart=3,--disable-warnings" --include-cov ${{ steps.files.outputs.added_modified_renamed }}

I am thinking of changing the default behavior to:

• running all tests whenever there is a code change
• if there are only docs changes, only the docs build are run

Or that we can make it such that:

• a label like "run CI" will trigger all test runs
• a label like "build docs" will trigger docs build
• otherwise nothing will be done

@oftfrfbf @ctuguinay : Any thoughts or suggestions?

Also, there was a label "Needs Complete Testing" that did not have any effect and was probably a relic from long time ago. I have removed it.

[oftfrfbf]

Sorry for the slow response, I have been thinking about this and some other things.

I have not triggered any doc builds yet so I am unfamiliar with the process.

I think that if you are struggling to keep your github-action-hours under some limit per month then it is worth while to keep the fail-fast and targeted testing structure in place. If you are not constrained by the test computing resources, then keeping everything as simple as possible is my recommendation and in that case just run the full test suite each time. Less lines of code in the testing infrastructure means more reliable results. Developers should in most cases be approaching pull requests with already tested code, by allowing them to only test small segments of the code we will instead be allowing them to introduce some very difficult to debug problems.

Running the full test suite each time also allows us to enforce a contract where if you break something you need to fix it.

...

These are some other unsolicited test recommendations as well:

Some of the test resources are pretty large, especially the ek60 and ek80 resources. It might be worth finding replacements for test files that have the same required and unique test attributes but are just smaller file sizes. I can can scan the S3 noaa-wcsd-pds bucket to find files if needed, but there should be a goal to keep the test suite small so that developers can more eaily run the full suite as needed. If the full test suite could run in <5 minutes then it would feel more accessible and useful.

Here is my latest scan of asset sizes by directory:

https://github.com/oftfrfbf/echopype-test-data/releases/tag/2024.12.23.10.10

...

Moving away from docker containers. We are going to soon be able to use pooch to get the test assets in place as needed via #1263, one other goal might be to completely remove the required docker integration. I have been working in other projects where I have been able to successfully create a mocked up s3 bucket, copy local pooch files to the mocked up bucket, run echopype to do data processing where I then create a zarr store, copy that zarr store to the mocked up s3 bucket, and then read directly from that bucket via xarray/zarr/s3fs to make assertions with pytest.

https://github.com/CI-CMG/water-column-sonar-processing/blob/aa9133f5e187a1517a224ba1416f6fdaa1148483/tests/processing/test_raw_to_zarr.py#L24

I think theoretically we should be able to migrate away from docker and developers could run the full test suite with only pytest. s3fs requires the threadedmotoserver, and all the aws stuff only ends up needing the "@mock_aws" decorator. I will note though that I have only been able to get this working for s3fs through v2024.2.0, it will require some more dev work.