#304 unify KeyboardInterrupt behaviour for watch commands
Merged by mikem. Opened by tkopecek.
tkopecek/koji issue302  into  master

Download 304.patch

Related to https://pagure.io/koji/issue/302

Why not just this?

https://github.com/mikem23/koji-playground/commits/issue302b

1 new commit added

  • fix return value

I would say, that it is better to behave same in all similar cases. There is no strong argument for, just to be more intuitive.

Question is if sleep(60) should be same as options.poll_interval, but I would say so from the same reason.

Question is if sleep(60) should be same as options.poll_interval, but I would say so from the same reason.

hm... The default poll_interval is 6s I think. The wait_repo handler used 60 because repo regens aren't particularly fast. OTOH, sometimes they can be and that sounds more consistent.

I would say, that it is better to behave same in all similar cases. There is no strong argument for, just to be more intuitive.

I think my version does that. Why wrap the list-history handler in a try..except to catch KeyboardInterrupt, when we can just do that at the top level?

rebased

Makes sense - I've rewritten it.

unit test fails. watch_tasks also returns 1 if one of the tasks fails. We need to preserve that

Hmm, so we probably want to create special exit code for KeyboardInterrupt. I hasn't been able to find anything 'standard' enough. Closest is bash's 130 (http://www.tldp.org/LDP/abs/html/exitcodes.html) Any better idea?

Do we need to return a special code? I'm not sure if anyone cares to distinguish between the two cases.

Note that python itself exits with code 1 if KeyboardInterrupt is not caught by the script.

rebased

Misunderstood the problem. I've returned that value and rebased to current master.

rebased

rebased

rebased

Some more comments?

So... this is very strange.

When I run make test on your branch, a bunch of unit tests are missed, dropping us down to 12% coverage. However, I cannot see any reason why this would be the case. The only changes on the branch are in cli/koji.

I've rechecked this several times, even using a fresh clone.

Here's where I am:

aaf6fa8 (HEAD -> issue302, tkopecek/issue302) fix return value for watch_tasks
aa80a63 unify KeyboardInterrupt behaviour for watch commands
882316c PR#336 use alabaster theme for docs

If I check out 882316c, the tests run as expected. At aa80a63, there is a loss of coverage and a failure. At aaf6fa8, the failure is gone, but the loss of coverage remains the same.

Less tests are being run.

-Ran 355 tests in 12.153s
+Ran 116 tests in 1.321s

The loss of coverage is not in cli/koji, but spread out all over

AHHHH. Found it. There is a "Raise KeyboardInterrupt inner watch_tasks" test that now appears to terminate testing, presumably because the cli no longer catches it.

rebased here with unit test fixes:
https://github.com/mikem23/koji-playground/commits/issue302

I have filed a bug against the test harness

https://github.com/nose-devs/nose/issues/1040

Confirming that tests are fixed by this.

Commit f499a448 fixes this pull-request

Pull-Request has been merged by mikem@redhat.com

Metadata