Skip to content

Add application test for pctpairprotons#78

Merged
SimonRit merged 1 commit into
RTKConsortium:mainfrom
acoussat:pctpairprotons-test
Apr 16, 2026
Merged

Add application test for pctpairprotons#78
SimonRit merged 1 commit into
RTKConsortium:mainfrom
acoussat:pctpairprotons-test

Conversation

@acoussat
Copy link
Copy Markdown
Collaborator

Following this comment.

Copy link
Copy Markdown
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

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

👍

@SimonRit SimonRit self-requested a review April 16, 2026 11:13
Copy link
Copy Markdown
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

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

@acoussat acoussat force-pushed the pctpairprotons-test branch from 67fa3ec to 8841a6b Compare April 16, 2026 11:46
@acoussat
Copy link
Copy Markdown
Collaborator Author

acoussat commented Apr 16, 2026

The Python test fails because uproot is missing. Can you add an optional dependency https://github.com/RTKConsortium/RTK/blob/main/pyproject.toml#L171?

Done!

Add then the option --group test on the line https://github.com/acoussat/PCT/blob/pctpairprotons-test/.github/workflows/build-test-package.yml#L51,

We checked together with @axel-grc and it does not work, and it is not how it is currently done in RTK. I have decided to follow RTK and just copy the dependencies there. Is it something we ought to change in RTK?

see https://docs.openrtk.org/en/latest/INSTALLATION.html#testing.

I copied and adapted the text from RTK, but I noticed that the command pytest test/ does not work, it should either be pytest/*.py, or rename all the test files to match test_*. I went for the first option, and I believe the doc in RTK should be corrected. @axel-grc do you want me to do it?

@acoussat acoussat requested a review from SimonRit April 16, 2026 11:51
Copy link
Copy Markdown
Collaborator

@SimonRit SimonRit left a comment

Choose a reason for hiding this comment

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

Good as is but it might be worth considering uproot a mandatory in the future.

@SimonRit SimonRit merged commit 4518bc1 into RTKConsortium:main Apr 16, 2026
14 checks passed
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.

2 participants