Skip to content

Conversation

@cadenmyers13
Copy link
Contributor

closes #46

This CI script sets RUN_EXAMPLES=1 on diffpy.cmi releases. If RUN_EXAMPLES isn't set, then the scripts aren't ran; therefore, this variable is only set on releases and not on PRs, meaning examples scripts won't be ran on every PR/push to a PR. If maintainers want to run this tests locally they run

RUN_EXAMPLES=1 pytest tests/test_examples.py

The caveat to this is that on releases, test_examples.py will be ran once, and all other tests will be ran twice. I don't think this is that big of an issue since the other tests run very quickly.

@cadenmyers13
Copy link
Contributor Author

@sbillinge ready for review, this will fail tests until the copy_examples PR is merged because of the python 3.14 issue. However, I tested this on a PR on my fork and it behaved as expected: cadenmyers13#9

@sbillinge
Copy link
Contributor

I don't love this fix. I think it is a bit of an overkill to set an environment variable for this purpose. Why don't we just make it that the run examples only runs on workflow_dispatch and we add this to the release checklist for diffpy.cmi?

@cadenmyers13
Copy link
Contributor Author

@sbillinge The challenge I'm running into with this is still using the release script _test-on-pr.yml while not having it run test_examples.py. The _test-on-pr.yml release script automatically runs pytest on every test file by running pytest --cov. I see three options.

Option 1 is the environment variable.

Option 2 is separately maintaining a test-on-pr.yml where the test_examples.py is ignored, and a separate workflow file (call it run-examples.yml) is only triggered on manual dispatch. In this separate workflow file, we only run pytest on test_examples.py. This option seems like it would create more maintenance.

Option 3 (most recent changes) is to add PYTEST_ADDOPTS. This append the pytest --cov command with --ignore=tests/test_examples.py. Then still have run-examples.yml which will be added to the release checklist.

@sbillinge
Copy link
Contributor

Is it possible to run a test on a file that has a non-standard name? Could we just change the name of the file from test_examples.py to validate_examples.py so that pytest doesn't run it. But then we have a separate CI file that on workflow_dispatch runs a snake-test on python -m pytest validators/validate_examples.py? Could something like that work? I would suggest to hand-write that workflow and keep it in the .github dir

@cadenmyers13
Copy link
Contributor Author

@sbillinge I like that idea. I'll implement it.

@cadenmyers13
Copy link
Contributor Author

@sbillinge ready for review. I tried to mimmic the release scripts workflow for the manual trigger: https://github.com/scikit-package/release-scripts/blob/v0/.github/workflows/_tests-on-pr.yml

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

this looks good.

Did you test that it works?

I think we should maybe move validate_examples.py to a directory called validators as it is not a file that does unit tests.

This isn't running them on all platforms and python versions, right? or it does? Which do we want?

@cadenmyers13
Copy link
Contributor Author

@sbillinge the examples only run on ubuntu and python 3.13 as is. moving this test inside validators doesn't work because it uses the temp dir fixture as defined in conftest. We could just move this fixture to the top of the file though. I think that should work.

@cadenmyers13
Copy link
Contributor Author

@cadenmyers13
Copy link
Contributor Author

This should pass all tests when the other PR is merged

@sbillinge
Copy link
Contributor

This should pass all tests when the other PR is merged

I merged the other PR and reran hte tests but no luck. Please could you take another look?

@sbillinge sbillinge merged commit a893e67 into diffpy:main Nov 1, 2025
2 of 4 checks passed
@sbillinge
Copy link
Contributor

I merged this anyway, but we have to figure out why tests are failing and fix it with some urgency....

@cadenmyers13 cadenmyers13 deleted the run-ex-on-releases branch November 3, 2025 14:47
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.

tests: remove tests on all sub-projects?

2 participants