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

Make PanBase hermetic across tests by clearing _config each time #249

Merged
merged 2 commits into from
Dec 25, 2017

Conversation

jamessynge
Copy link
Contributor

A simple fix for issue #246.

@jamessynge jamessynge requested a review from wtgee December 24, 2017 20:45
@codecov
Copy link

codecov bot commented Dec 24, 2017

Codecov Report

Merging #249 into develop will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #249      +/-   ##
===========================================
+ Coverage     79.8%   79.85%   +0.04%     
===========================================
  Files           42       42              
  Lines         3204     3206       +2     
  Branches       415      415              
===========================================
+ Hits          2557     2560       +3     
  Misses         503      503              
+ Partials       144      143       -1
Impacted Files Coverage Δ
pocs/base.py 96.96% <100%> (+0.19%) ⬆️
pocs/observatory.py 85.61% <0%> (-0.67%) ⬇️
pocs/dome/__init__.py 100% <0%> (+8.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 160f3f2...3d36c26. Read the comment docs.

pocs/base.py Outdated
@@ -10,6 +10,16 @@
_config = None


def reset_config_for_test():
Copy link
Member

Choose a reason for hiding this comment

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

What about just making this an option that is passed to PanBase, like ignore_local_config? I can imagine there might be a situation where someone needs to reset the config for whatever reason. Or just change this function to reset_config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are many instances of PanBase in a running POCS system, and it doesn't make sense (to me) for them to have different global configs.

I suggested elsewhere that we might have a PanConfig object that gets passed around, and represents whatever config has been loaded by the creator of POCS and Observatory. This fix is designed to allow us to make a decision about that latter, yet still get closer to hermetic tests now.

So, I'll rename to reset_global_config, with a comment that it is intended for use in tests, but might have other uses.

Copy link
Member

@wtgee wtgee left a comment

Choose a reason for hiding this comment

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

👍

@wtgee wtgee merged commit 37bc82a into panoptes:develop Dec 25, 2017
@wtgee wtgee mentioned this pull request Dec 29, 2017
@jamessynge jamessynge deleted the issue-246-simple-hermetic-config branch January 1, 2018 02:33
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