Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TRR tools in user scripts #155

Merged
merged 8 commits into from
Oct 22, 2024
Merged

TRR tools in user scripts #155

merged 8 commits into from
Oct 22, 2024

Conversation

dmousadi
Copy link
Contributor

@dmousadi dmousadi commented Oct 9, 2024

Added the directory with the camera test scripts for the TRR. Ignore the first two commits (accidentally added it as submodule). There is a readme file that explains how the tests work.

@jlenain
Copy link
Collaborator

jlenain commented Oct 9, 2024

Could you please lint the code using our pre-commit hooks, such that the CI passes? Thanks!

@dmousadi
Copy link
Contributor Author

Done! Let me know if there is more I need to change or if I did something wrong.

@jlenain
Copy link
Collaborator

jlenain commented Oct 21, 2024

Hi @dmousadi ,
Sorry for the delay, I was busy with the CADS school last week.
The thing is your PR currently fails in CI, see the last checks, e.g. https://github.com/cta-observatory/nectarchain/actions/runs/11290081940/job/31499460359?pr=155. The code still needs some linting before being acceptable for merge.

Also, I do not understand why #155 (comment) is marked as solved.

Thanks!

@dmousadi
Copy link
Contributor Author

Hi! Sorry, I thought I deleted that before. I was not familiar with pre-commit but I think it's fine now? Let me know if I have to change something now.

@jlenain
Copy link
Collaborator

jlenain commented Oct 22, 2024

Hi @dmousadi !
No worry, and thanks for this! I will launch again the workflow in CI, and see if it passes.

@jlenain
Copy link
Collaborator

jlenain commented Oct 22, 2024

OK, the linting part passes, but not pytest (see https://github.com/cta-observatory/nectarchain/actions/runs/11459609758/job/31884930865?pr=155). I think this is due to the fact that your PR contains directories and files with test in their name. Let me try to exclude the user_scripts altogether from unit tests.

@jlenain
Copy link
Collaborator

jlenain commented Oct 22, 2024

See #156

@jlenain
Copy link
Collaborator

jlenain commented Oct 22, 2024

@dmousadi , could you please allow me to commit and push into your fork? I need to rebase from upstream to solve #156 for this PR. Thanks!

@dmousadi
Copy link
Contributor Author

Okay, I added you as a collaborator

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.00%. Comparing base (b306f30) to head (cae51ac).
Report is 30 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
+ Coverage   40.76%   49.00%   +8.23%     
==========================================
  Files          65       64       -1     
  Lines        4452     4561     +109     
==========================================
+ Hits         1815     2235     +420     
+ Misses       2637     2326     -311     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jlenain jlenain merged commit 2329514 into cta-observatory:main Oct 22, 2024
11 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