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-24920: improve butler CLI mocking #244

Merged
merged 1 commit into from May 15, 2020
Merged

DM-24920: improve butler CLI mocking #244

merged 1 commit into from May 15, 2020

Conversation

n8pease
Copy link
Contributor

@n8pease n8pease commented May 13, 2020

No description provided.

Copy link
Member

@timj timj 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 interesting. I haven't really used mocks before.

I think there were a couple of other per-line comments on the previous PR that could also be fixed now.


def test_repoAndOutput(self):
"""Test the most basic required arguments, repo and output run"""
expected = makeExpectedKwargs(repo="here", output_run="out")
self.run_test(["ingest-raws", "here", "--output-run", "out"], expected)
self.run_test(["ingest-raws", "here",
Copy link
Member

Choose a reason for hiding this comment

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

Is this change needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not necessary, but makes this test function look like the rest of the test functions in the suite, making it so that in the tuple of parameters each option+value is on a separate line (the first line is consistently a special case, subcommand+repo), which IMO is easier to read.

@@ -43,6 +44,9 @@ def makeExpectedKwargs(**kwargs):

class Suite(unittest.TestCase):

def setUp(self):
sys.modules["lsst.obs.base.script.ingestRaws"] = lsst.obs.base.cli.test.ingestRaws
Copy link
Member

Choose a reason for hiding this comment

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

I'd feel better if there was a teardown that reversed this since sys.modules is a global.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I've used unittest.patch for this recently, I'll look into using it for this situation.

@n8pease n8pease force-pushed the tickets/DM-24920 branch 2 times, most recently from 9cb80c9 to f260fc9 Compare May 14, 2020 17:24
@n8pease n8pease merged commit 3425666 into master May 15, 2020
@timj timj deleted the tickets/DM-24920 branch June 25, 2020 22:49
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