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

Generaloption: handle option-as-value #185

Merged
merged 21 commits into from
Oct 10, 2015

Conversation

stdweird
Copy link
Member

Fixes #184
Requires #193

@hpcugentbot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/vsc-base-pr-builder/312/
Test FAILed.

@stdweird
Copy link
Member Author

@boegel this seems to works, but no unittests were added yet (and the current ones fail due to double logger somewhere, matser is fine)

@hpcugentbot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/vsc-base-pr-builder/313/
Test FAILed.

def parse_args(self, args=None, values=None):
"""Keep a copy of the original arguments for callback / option processing"""
# seems like self._get_args returns a copy, but better safe then sorry here
self.orig_rargs = self._get_args(args)[:]
Copy link
Member

Choose a reason for hiding this comment

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

I think if you make this super(..., self)._get_args(args), then you can also remove the premature return I added in self._get_args get_env_options

Copy link
Member

Choose a reason for hiding this comment

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

well, close, super doesn't work because OptionParser doesn't derive from object

all tests pass with stdweird#9

@hpcugentbot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/vsc-base-pr-builder/316/

@hpcugentbot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/vsc-base-pr-builder/317/

@stdweird
Copy link
Member Author

stdweird commented Oct 8, 2015

@boegel should be fine now

@hpcugentbot
Copy link

All tests PASSed.

See https://jenkins1.ugent.be/job/vsc-base-pr-builder/343/console for more details.

@boegel
Copy link
Member

boegel commented Oct 8, 2015

Since #193 is merged in here, that should be reviewed/merged first, and this one should then be re-evaluated.

@boegel
Copy link
Member

boegel commented Oct 9, 2015

@stdweird: please sync with master to trigger an updated diff/retest?

@stdweird stdweird force-pushed the go_nooptionasvalue branch from 627000f to fe624d0 Compare October 9, 2015 11:39
@hpcugentbot
Copy link

All tests PASSed.

See https://jenkins1.ugent.be/job/vsc-base-pr-builder/351/console for more details.

@stdweird stdweird force-pushed the go_nooptionasvalue branch from fe624d0 to a53ef03 Compare October 9, 2015 14:44
@hpcugentbot
Copy link

All tests PASSed.

See https://jenkins1.ugent.be/job/vsc-base-pr-builder/354/console for more details.

@hpcugentbot
Copy link

All tests PASSed.

See https://jenkins1.ugent.be/job/vsc-base-pr-builder/355/console for more details.

self.mock_stderr(True) # reset
self.assertErrorRegex(SystemExit, '.*', TestOption1, go_args=args)
stderr = self.get_stderr()
self.assertTrue(msg in stderr)
Copy link
Member

Choose a reason for hiding this comment

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

add self.mock_stderr(False) here to disable the mocking (so we can actually see something if something is broken hard :)

maybe the self.assertErrorRegex during mocking isn't a good idea either, will we see the actual failed test since stderr is being mocked?

maybe this is a good place for try/except/finally instead of self.assertErrorRegex:

def _match_testoption1_sysexit(self, args, msg):
    """Check whether parsing supplied arguments with TestOption1 results in SystemExit error being raised."""
    system_exit = False
    self.mock_stderr(True)  # reset
    # purposely not using self.assertErrorRegex, since we're mocking stderr...
    try:
        TestOption(go_args=args)
    except SytemExit:
        system_exit = True
    finally:
        self.mock_stderr(False)
    self.assertTrue(system_exit)

Copy link
Member

Choose a reason for hiding this comment

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

ah, yes, and add the self.assertTrue(msg in stderr) too

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't understand. isn't the setUp / tearDown supposed to do this? also don't understand the reamkr wrt exceptions and stderr.

Copy link
Member

Choose a reason for hiding this comment

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

While you're mocking stderr, you won't get any output on stderr anymore.

The tearDown will disable the mocking, sure, but it's cleaner to do that in here (since you may be calling this method several times in a single test).

@hpcugentbot
Copy link

All tests PASSed.

See https://jenkins1.ugent.be/job/vsc-base-pr-builder/361/console for more details.

@stdweird
Copy link
Member Author

@boegel not going to merge #199 since i have no clue what else is in there and i'm not going to review it

boegel added a commit that referenced this pull request Oct 10, 2015
Generaloption: handle option-as-value
@boegel boegel merged commit 83e8dc4 into hpcugent:master Oct 10, 2015
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.

3 participants