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 test for spectrum urls #404

Merged
merged 9 commits into from
Oct 17, 2023
Merged

Conversation

kelle
Copy link
Collaborator

@kelle kelle commented Sep 19, 2023

New test to make sure spectrum urls don't give 404. This test takes some time....not sure we want to keep it, but it's useful at the moment. Related to #347.

Link to relevant issue: Closes #369

@dr-rodriguez
Copy link
Collaborator

My worry is that this may be attempting to fetch thousands of spectra for each time the tests runs, which is why it takes a longer time.
Perhaps a better approach is that this test exists, but is in a separate file and there is a separate github action for a periodic (once per week maybe) execution as opposed to with every commit.

@kelle
Copy link
Collaborator Author

kelle commented Sep 21, 2023

YES! Here's how to do it: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule

What should we call the file? still test_? If we do that, it will be run by default by pytest. So maybe a different name?

@dr-rodriguez
Copy link
Collaborator

We could change the pytest defaults, or just call it something like scheduled_jobs.py or something similar

@kelle
Copy link
Collaborator Author

kelle commented Sep 21, 2023

scheduled_tests?

@dr-rodriguez
Copy link
Collaborator

I don't know if the pytest defaults look at anything that starts with test_ or just have test somewhere in it. We probably should explicitly have a pytest.ini file to control that. There's also a way to set markers on pytest so we could have slow/fast tests and configured the current tests to use the fast tests and the scheduled ones to use the slow ones.
But if you want to be safe and not add any of that, maybe just avoid test in the filename.

@kelle
Copy link
Collaborator Author

kelle commented Sep 22, 2023

Here's the documentation for pytest autodiscovery:
https://docs.pytest.org/en/6.2.x/goodpractices.html#conventions-for-python-test-discovery

My first choice is to make a file/test which just doesn't get auto-discovered but still lives in tests/

tests/test_data.py Outdated Show resolved Hide resolved
@kelle kelle requested a review from dr-rodriguez October 13, 2023 21:10
@kelle
Copy link
Collaborator Author

kelle commented Oct 13, 2023

This is ready for re-review. It's just the tests. Once it's merged, I'd like to run the action, make sure it works with all the broken URLs.

Then I can work on #409 to actually fix the URLs.

@kelle kelle mentioned this pull request Oct 13, 2023
@kelle kelle merged commit 65e36e3 into SIMPLE-AstroDB:main Oct 17, 2023
1 check passed
@kelle kelle deleted the test-spectra-urls branch October 17, 2023 20:26
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.

Make test which makes sure all the spectra URLs are valid
2 participants