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

DM-11614: Reset filter list before running each test #48

Merged
merged 1 commit into from Aug 23, 2017

Conversation

timj
Copy link
Member

@timj timj commented Aug 19, 2017

Without this, in pytest, when CFHT tests run they define a "u" filter,
and when the DECam tests run they fail because the "u" filter
has already been defined.

@timj timj requested a review from parejkoj August 19, 2017 19:00
@parejkoj
Copy link
Collaborator

Although it's probably not a bad idea to do this here, I think the better solution is the one employed in DM-9297, resetting the filters in the obs package, before they are (re)defined. See the discussion when I fixed this for lsstSim here:

https://jira.lsstcorp.org/browse/DM-9297

@timj
Copy link
Member Author

timj commented Aug 22, 2017

This explains why lsstSim worked and Decam failed. This is blocking the pytest merge and I'm not sure I'm up to speed enough to patch obs_decam.

@parejkoj
Copy link
Collaborator

I'd guess that adding afwImageUtils.resetFilters() to decamMapper.py:60 in obs_decam and to megacamMapper.py:57 in obs_cfht should do it.

@timj
Copy link
Member Author

timj commented Aug 22, 2017

I have made the suggested changes to obs_decam and obs_cfht but I am getting push back from @ctslater who doesn't think there should be any special casing and that the tests should run independently outside of pytest. This can be made to work but means we lose the ability to run pytest on its own. I'd prefer this patch be accepted to the jointcal tests to reset them if we aren't going for the obs_ solution.

@timj
Copy link
Member Author

timj commented Aug 22, 2017

@parejkoj I've reinstated the fix for the tests. The fix is in all of them for consistency and so as not to rely on the behavior of a particular mapper.

@parejkoj
Copy link
Collaborator

I wonder if we shouldn't put the resetFilter calls in setUp instead of setupClass? The latter protects from cross-obs runs, but the former would also protect from within-obs runs, I think.

@timj
Copy link
Member Author

timj commented Aug 22, 2017

It shouldn't do any harm. I'll try it. I hope that setUpClass can't run and be followed by a test from a different class but now I'm wondering about that...

Without this, in pytest,  when CFHT tests run they define a "u" filter,
and when the DECam tests run they fail because the "u" filter
has already been defined.
@timj timj merged commit 59ec720 into master Aug 23, 2017
@ktlim ktlim deleted the tickets/DM-11614 branch August 25, 2018 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants