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

Remove some agasc supplement bad star tests #400

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Conversation

jeanconn
Copy link
Contributor

Description

Remove some agasc supplement bad star tests .

This does not address the issue that maybe this test_agasc_supplement should be removed or updated to work with a static or versioned supplement.

Fixes issue that the asserts in the test were not valid after removing some stars from the bad list.

Interface impacts

Testing

Unit tests

  • Mac arm64
(ska3) flame:proseco jean$ git rev-parse head
a25c3a78e4321590f33082062b2d461607dfbb5e
(ska3) flame:proseco jean$ pytest 
========================================================================================== test session starts ==========================================================================================
platform darwin -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0
rootdir: /Users/jean/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collected 167 items                                                                                                                                                                                     

proseco/tests/test_acq.py .....................................                                                                                                                                   [ 22%]
proseco/tests/test_catalog.py ..........................................                                                                                                                          [ 47%]
proseco/tests/test_core.py ...........................                                                                                                                                            [ 63%]
proseco/tests/test_diff.py ......                                                                                                                                                                 [ 67%]
proseco/tests/test_fid.py ...............                                                                                                                                                         [ 76%]
proseco/tests/test_guide.py ....................................                                                                                                                                  [ 97%]
proseco/tests/test_mon_full_cat.py ....                                                                                                                                                           [100%]

=========================================================================================== warnings summary ============================================================================================
../../miniconda_m1/envs/ska3/lib/python3.11/site-packages/tables/node.py:251: 1 warning
proseco/proseco/tests/test_acq.py: 35 warnings
proseco/proseco/tests/test_catalog.py: 52 warnings
proseco/proseco/tests/test_core.py: 17 warnings
proseco/proseco/tests/test_fid.py: 18 warnings
proseco/proseco/tests/test_guide.py: 109 warnings
proseco/proseco/tests/test_mon_full_cat.py: 5 warnings
  /Users/jean/miniconda_m1/envs/ska3/lib/python3.11/site-packages/tables/node.py:251: DeprecationWarning: `alltrue` is deprecated as of NumPy 1.25.0, and will be removed in NumPy 2.0. Please use `all` instead.
    self._v_objectid = self._g_open()

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================================== 167 passed, 237 warnings in 21.53s

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

No functional testing.

@jeanconn jeanconn requested a review from javierggt August 19, 2024 17:09
@javierggt
Copy link
Contributor

I would just remove the test. That test does nothing here, and the test itself is trivial. It does not even test something that conceivably proseco would depend on.

@jeanconn
Copy link
Contributor Author

"It does not even test something that conceivably proseco would depend on". well - it tests that there is a bad_star_set loaded from a supplement and it has some stars in it.

@javierggt
Copy link
Contributor

The whole point of unit testing is that each package is tested on its own, and other packages that depend on them do not have to repeat the tests. Then, when the package changes, the tests change accordingly. When tests are replicated in other places, then one has to go and fix them (like now).

I can imagine a case where one adds a test of a dependency as a kind of precondition, so when other tests fail it is clear that there is a root cause. This is not that case. This is an unnecessary test that is already causing us to make this PR that should have never happened.

@jeanconn
Copy link
Contributor Author

My point was that this is actually a test that provides coverage of the loading of the bad star list from proseco's little wrapper https://github.com/sot/proseco/blob/master/proseco/characteristics.py#L78 .

@jeanconn
Copy link
Contributor Author

But I suppose that gets coverage from almost every other test, just no confirmation that stars were actually loaded.

@jeanconn jeanconn merged commit 2814d21 into master Aug 21, 2024
2 checks passed
@jeanconn jeanconn deleted the fewer-bad-stars branch August 21, 2024 19:41
@javierggt javierggt mentioned this pull request Nov 7, 2024
@javierggt javierggt mentioned this pull request Nov 19, 2024
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