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

Fix pytests #59

Merged
merged 18 commits into from
Sep 28, 2023
Merged

Fix pytests #59

merged 18 commits into from
Sep 28, 2023

Conversation

stscirij
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@jotaylor jotaylor left a comment

Choose a reason for hiding this comment

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

@stscirij can you remind me how to run the tests locally?

@stscirij
Copy link
Collaborator Author

@stscirij can you remind me how to run the tests locally?

If you go to the tests directory, you can either do
pytest
to run all the tests under pytest. This is sometimes awkward to interpret as the stdout isn't printed.
Alternatively, to run each test individually, you can do, from the tests directory
ipython
import test_wrapper
a = test_wrapper.TestWrapper()
a.test_av321()
a.test_av456()
a.test_hd104237e()
a.test_v_hk_ori()
Make sure you have installed pandas 2.0 or later in your local environment

@tapastro
Copy link
Collaborator

@stscirij can you remind me how to run the tests locally?

If you go to the tests directory, you can either do pytest to run all the tests under pytest. This is sometimes awkward to interpret as the stdout isn't printed.

You can try pytest -s or pytest --capture=no to see stdout, if that helps.

@jotaylor
Copy link
Collaborator

@stscirij After some trail & error, I fixed all of the issues in the tests that I could. However, with some recent alias updates the names of the AV targets are now out of date. av321 is now av-321 and av456 is av-456. Can you update the following:

  1. names of targets in test_wrapper.py
  2. names of target directories in Box folders
  3. names of HLSPs in Box folders
    I think with these changes things should start working!

@stscirij
Copy link
Collaborator Author

stscirij commented Sep 26, 2023

@stscirij After some trail & error, I fixed all of the issues in the tests that I could. However, with some recent alias updates the names of the AV targets are now out of date. av321 is now av-321 and av456 is av-456. Can you update the following:

  1. names of targets in test_wrapper.py
  2. names of target directories in Box folders
  3. names of HLSPs in Box folders
    I think with these changes things should start working!

@jotaylor Code is still trying to read non-existent ullyses-utils file pd_targetinfo.json in get_coords(). Maybe tag ullyses-utils with all the new aliases files as version 2 so that ullyses can be in sync?

@jotaylor
Copy link
Collaborator

@jotaylor Code is still trying to read non-existent ullyses-utils file pd_targetinfo.json in get_coords(). Maybe tag ullyses-utils with all the new aliases files as version 2 so that ullyses can be in sync?

Oops, that file was not meant to be deleted 😅 I added it back into utils, so if you do a fresh install it should good.

@jotaylor
Copy link
Collaborator

Okay, I think things are looking as they should now for the most part. The tests are failing because the products are different than those we delivered in DR6, but that's expected. I also changed the format of the RA/Dec to float instead of string.

However, before we can merge, there is a typo in the python 3.10 run. It's trying to install python 3.1 and failing. @stscirij can you change it to 3.10? I'm not sure if that's done somewhere in the actual code, or in the git settings in the browser.

@jotaylor
Copy link
Collaborator

Green check marks?! Haven't seen those in a while. I think we are good to merge! 🥳

@jotaylor jotaylor merged commit e3108a1 into spacetelescope:main Sep 28, 2023
3 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.

4 participants