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

Add fixed_time_pickoff unit test #399

Merged

Conversation

slwatkins
Copy link
Member

@slwatkins slwatkins commented Oct 21, 2022

I wrote a simple unit test for all of the various cases of fixed_time_pickoff, and added it to the testing suite. I have a couple of questions before marking this as ready to review.

  1. Are we okay with using the simple ndarrays for this test as I did? The outputs are more easily understand than using LEGEND test data, but perhaps there was a decision somewhere to always use test data.
  2. This doesn't actually do anything to the code coverage, since the Python code itself isn't actually run due to the JIT compilation. Do we want to exclude these numba-wrapped functions from the coverage calculations? I'm not sure of a way of including them in the coverage other than creating two versions of each (like this: https://stackoverflow.com/a/47970347), which I'd be surprised if we'd want to do that.

I'm happy to start working through other tests as needed, but just want to make sure I understand the test writing conventions before going through more!

EDIT: I also accidentally created this branch off of the one in #394, so that should be merged before this.


Before submitting a pull request, please make sure you've read and understood the pygama developer's guide: https://pygama.readthedocs.io/en/latest/developer.html. In particular, do not forget to:

  • Conform to our coding conventions
  • Update existing or add new tests
  • Update existing or add new documentation
  • Address any issue reported by GitHub checks

@gipert gipert requested a review from iguinn October 22, 2022 15:26
@gipert gipert added tests Testing the package dsp Digital Signal Processing labels Oct 22, 2022
@gipert
Copy link
Member

gipert commented Oct 22, 2022

Hi Sam, I fully agree we should test processors with NumPy arrays!

Let's discuss test coverage with Numba in #241.

Copy link
Collaborator

@iguinn iguinn left a comment

Choose a reason for hiding this comment

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

Looks good! For these kinds of tests that are checking multiple different functionalities, it might be nice to have a list of the tests performed in the docstring or something like that. Having it all in one place I think will make it easier to see if there is anything missing.

Can you also test the __wrapped__ suggestion from #241, and see if that triggers the coverage of this function?

@slwatkins slwatkins mentioned this pull request Oct 24, 2022
5 tasks
@slwatkins
Copy link
Member Author

@iguinn I can add a commit where I quickly explain the tests in the docstring, but wanted to check what people thought about how we want to use the __wrapped__ attribute before doing much more.

Also, I looked at the code coverage job, and it looked like it actually failed at the "Upload Coverage to codecov.io" step, but the job is marked as successful (see error at bottom of this reply). We might need to figure out a way to mark this is a failure and have the workflow be re-run. Maybe @gipert can take a look?

Run codecov/codecov-action@v3
==> linux OS detected
https://uploader.codecov.io/latest/linux/codecov.SHA256SUM
==> SHASUM file signed by key id 806bb28aed779869
==> Uploader SHASUM verified (20f9c9d78483fce977b6cc39e231a734a23bcd36f4d536bb7355222fb88d02bc  codecov)
==> Running version latest
==> Running version v0.3.2
/home/runner/work/_actions/codecov/codecov-action/v3/dist/codecov -n  -Q github-action-3.1.1 -C 96d0268177c42a5e501a050d8beb8ac58f24fb72
[2022-10-24T20:48:27.602Z] ['info'] 
     _____          _
    / ____|        | |
   | |     ___   __| | ___  ___ _____   __
   | |    / _ \ / _` |/ _ \/ __/ _ \ \ / /
   | |___| (_) | (_| |  __/ (_| (_) \ V /
    \_____\___/ \__,_|\___|\___\___/ \_/

  Codecov report uploader 0.3.2
[2022-10-24T20:48:27.613Z] ['info'] => Project root located at: /home/runner/work/pygama/pygama
[2022-10-24T20:48:27.619Z] ['info'] -> No token specified or token is empty
[2022-10-24T20:48:27.839Z] ['info'] Running coverage xml...
[2022-10-24T20:48:29.460Z] ['info'] Searching for coverage files...
[2022-10-24T20:48:29.540Z] ['info'] Warning: Some files located via search were excluded from upload.
[2022-10-24T20:48:29.540Z] ['info'] If Codecov did not locate your files, please review https://docs.codecov.com/docs/supported-report-formats
[2022-10-24T20:48:29.541Z] ['info'] => Found 1 possible coverage files:
  coverage.xml
[2022-10-24T20:48:29.541Z] ['info'] Processing /home/runner/work/pygama/pygama/coverage.xml...
[2022-10-24T20:48:29.572Z] ['info'] Detected GitHub Actions as the CI provider.
[2022-10-24T20:48:29.575Z] ['info'] Pinging Codecov: https://codecov.io/upload/v4?package=github-action-3.1.1-uploader-0.3.2&token=*******&branch=tests%2Fcreate_fixed_time_pickoff_test&build=3316033460&build_url=https%3A%2F%2Fgithub.com%2Flegend-exp%2Fpygama%2Factions%2Fruns%2F3316033460&commit=96d0268177c42a5e501a050d8beb8ac58f24fb72&job=pygama&pr=399&service=github-actions&slug=legend-exp%2Fpygama&name=&tag=&flags=&parent=
[2022-10-24T20:48:29.915Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 404 - {'detail': ErrorDetail(string='Unable to locate build via Github Actions API. Please upload with the Codecov repository upload token to resolve issue.', code='not_found')}

@gipert gipert force-pushed the tests/create_fixed_time_pickoff_test branch from 53d2f55 to c916660 Compare October 25, 2022 08:00
@gipert
Copy link
Member

gipert commented Oct 25, 2022

These upload failures seem to occur with tokenless uploads: codecov/codecov-action#557 (comment)

It should be fixed now. The CI will fail if uploading fails, from now on.

Let's wee whether the Codecov bot shows up now.

@slwatkins slwatkins marked this pull request as ready for review October 25, 2022 13:36
@gipert
Copy link
Member

gipert commented Oct 25, 2022

Ok, the token does not work for forks. Those can only rely on tokenless uploads, which can fail sometimes and there's nothing we can do about it. Codecov devs will hopefully address the issue in the future.

@gipert gipert merged commit f8dda1a into legend-exp:main Oct 25, 2022
@slwatkins slwatkins deleted the tests/create_fixed_time_pickoff_test branch October 25, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dsp Digital Signal Processing tests Testing the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants