Opened 11 years ago

Closed 11 years ago

#9243 closed enhancement (fixed)

sage-doctest should use powers of 2 for return codes

Reported by: ddrake Owned by: mvngu
Priority: minor Milestone: sage-4.5.2
Component: doctest coverage Keywords:
Cc: wjp Merged in: sage-4.5.2.alpha1
Authors: Dan Drake, Willem Jan Palenstijn Reviewers: Willem Jan Palenstijn, Mitesh Patel
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges


Right now, sage-doctest uses these return codes:

# Return value in process exit code:
# 0: all tests passed
# 1: file not found
# 2: KeyboardInterrupt
# 3: doctest process was terminated by a signal
# 4: the doctesting framework raised an exception
# 100: failed doctests

In #8641 and #9224, we make sure that the return code gets passed on to the user, and for multiple files, we `or' the return codes together. It would be much nicer for the user if we used powers of 2 for return codes, so that it's easy to see exactly what happened.

I recommend we change 3->4, 4->8, and 100->128 in sage-doctest.

Attachments (2)

trac_9243.patch (3.8 KB) - added by ddrake 11 years ago.
apply to scripts repo
trac_9243_2.patch (1.1 KB) - added by wjp 11 years ago.
apply after trac_9243.patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 11 years ago by ddrake

  • Status changed from new to needs_review

Patch up, please review. This depends on #8641. I added an exit code of 16 for "bad options"; previously, sage-doctest exited with code 1 for both "file not found" and "bad options".

Changed 11 years ago by ddrake

apply to scripts repo

comment:2 Changed 11 years ago by ddrake

The new patch changes the absurdly accurate reporting of how long the tests took when you hit Ctrl-C; before we had "KeyboardInterrupt? -- interrupted after 2.2340259552 seconds!" and now it just uses one decimal place, instead of 10.

comment:3 Changed 11 years ago by mpatel

  • Cc wjp added

When #9316 merges, will we need to use 256, too, or use a different collection scheme?

comment:4 Changed 11 years ago by wjp

Thanks for adding me to CC.

I like this patch. There's one more instance, though: test_file in sage-ptest uses 5 as its own internal error code for unhandled exception. We should probably change that to 32. (And keep that one reserved.)

If #9316 is rebased on top of this one (which I can do), it will use error code 64 for timeouts.

All 8 available bits are then in use, so if we want to add more error conditions after that, we'll have to re-think this, or merge some error flags together.

Meta-comment: there are now three doctest related patches that I'm aware of, and that should probably all be merged. I would suggest this order: #9243, #9316, #8641.

comment:5 Changed 11 years ago by wjp

Meta-meta-comment: sorry, I'm blind. This patch already depends on #8641 of course. I'll rebase #9316 to apply after #8641 and #9243.

Changed 11 years ago by wjp

apply after trac_9243.patch

comment:6 Changed 11 years ago by wjp

I added a patch to address the internally used error code 5.

Positive review for the rest of the patch. Could someone review my small reviewer patch?

comment:7 Changed 11 years ago by mpatel

  • Authors set to Dan Drake, Willem Jan Palenstijn
  • Reviewers set to Willem Jan Palenstijn, Mitesh Patel
  • Status changed from needs_review to positive_review

To release manager: Apply both patches to 4.5.alpha4 + #8641.

comment:8 Changed 11 years ago by wjp

  • Milestone changed from sage-5.0 to sage-4.5.1

comment:9 Changed 11 years ago by ddrake

  • Merged in set to sage-4.5.2.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged both patches in 4.5.2.alpha1.

Note: See TracTickets for help on using tickets.