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

WIP : Fix Part 1 of issue 51, uses os.path.basename(sys.argv[0]) #55

Closed
wants to merge 1 commit into from

Conversation

saurabhkpatel
Copy link
Contributor

Need help for creating the test case for this change.

Need help for creating the testcase for this change.
@dbieber
Copy link
Member

dbieber commented Apr 9, 2017

To test a specific argv you can use mock.patch as shown here.
with mock.patch.object(sys, 'argv', ['progname']):

I would add the test to core_test.py, maybe as a second test after testFire named testFireDefaultName.
The test could use a filepath for sys.argv[0], include '--', '--trace' as args, and then do something like check that the basename shows up in the output.

Ideally there would be a way to test that name is being set appropriately, and then separately in trace_test we could test that the name is being used appropriately, but we don't have a good way to do the former.

@saurabhkpatel
Copy link
Contributor Author

@dbieber thanks for your suggestions :)
Never used mock in the python but this is the opportunity to learn from the community :)

I would write a new test method in core_test.py. My thoughts are similar like this.

  def testFireDefaultName(self):
    with mock.patch.object(sys, 'argv',
                           ['/github/python-fire/fire/base_filename.py']):
      output = fire.Fire(tc.Empty)
     assertIn("base_filename.py", output)    # would it work? 

I am not sure how to gather print output as return of fire.Fire(tc.Empty).

Any suggestions?

@dbieber
Copy link
Member

dbieber commented Apr 11, 2017

Once #54 is checked in you can use with self.assertOutputMatches to check the output of something like fire.Fire(tc.Empty). That should be checked in ~tomorrow.

@dbieber
Copy link
Member

dbieber commented Apr 11, 2017

^^ #54 is checked in now

@dbieber
Copy link
Member

dbieber commented Apr 12, 2017

Closing. Superseded by #59.

@dbieber dbieber closed this Apr 12, 2017
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.

None yet

2 participants