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

support mock QA with fitsio>1.0 (bytes vs. str) #634

Merged
merged 5 commits into from Jul 31, 2020
Merged

support mock QA with fitsio>1.0 (bytes vs. str) #634

merged 5 commits into from Jul 31, 2020

Conversation

sbailey
Copy link
Contributor

@sbailey sbailey commented Jul 30, 2020

This PR fixes #632 so that mock QA works with fitsio > 1.0 by adding extra bytes vs. str checks. It adds some trimmed down mock targets+truth test data and uses that to test mock QA. I have tested with with both fitsio/0.9.11 and fitsio/1.1.2 .

As a cleanup measure, this PR also wraps all test/make_test*.py code in if __name__ == "__main__": blocks so that importing the code (e.g. by some documentation scraper) doesn't accidentally trigger trying to regenerate the test datasets.

@sbailey sbailey requested a review from geordie666 July 30, 2020 19:00
@sbailey sbailey added this to In progress in 20.9 via automation Jul 30, 2020
Copy link
Contributor

@geordie666 geordie666 left a comment

Choose a reason for hiding this comment

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

This looks good to me...I can run all of the relevant code at NERSC without triggering any failures.

There may be cases for the mock QA in general that I'm not the best person to review. But, I independently tested passing bytes and unicode strings through QA for various parameters in the lightweight mocks files and everything passed.

@geordie666
Copy link
Contributor

I'll merge this tomorrow morning if you don't beat me to it.

@sbailey
Copy link
Contributor Author

sbailey commented Jul 31, 2020

Thanks for the additional checks. Merging now.

FWIW, the code previously passed unit tests, and now I'm going through the integration test and for anything that is failing there, I'm going back and making sure that there is a unit test that would have caught the problem. I think I am done with desitarget, but am now working on other repos.

@sbailey sbailey merged commit 59908a9 into master Jul 31, 2020
20.9 automation moved this from In progress to Done Jul 31, 2020
@sbailey sbailey deleted the mocktruth branch July 31, 2020 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
20.9
  
Done
Development

Successfully merging this pull request may close these issues.

mock QA failing with fitsio 1.x
2 participants