Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#24343 closed defect (fixed)

py3: miscellaneous fixes to sage.doctest

Reported by: embray Owned by:
Priority: major Milestone: sage-8.2
Component: python3 Keywords:
Cc: chapoton Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 2155133 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

These are most of the fixes I've made so far to the sage.doctest package. It's probably not exhaustive, but these are at least the main bug fixes that were needed to get it working (and for its own self-tests to pass, though that also requires fixes in other modules).

Change History (69)

comment:1 Changed 3 years ago by embray

  • Cc chapoton added

This won't be "ready for review" I think until some of its dependencies are dealt with first, but in the meantime if you want to try using these fixes it should prove helpful.

comment:2 Changed 3 years ago by git

  • Commit changed from fd139c9bca7cde3189262290cb6f904baaf02892 to ccd5e960503c867170f9bffd31364da35e962aca

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

589e7a2Slight reworking of the arguments to atomic_write:
9a4d548Slight correction to this docstring.
9398b57Miscellaneous fixes to tests in the doctest module.
5426bdbFix various minor encoding issues, and specify an encoding to use for writing the stats file.
a5663ccConvert these classes to 'new-style' classes for consistency with Python 3
18cdffcUse the restore_atexit context manager for this code to work on Python 3,
2203ca2Various fixes intended to ensure that opened files are closed explictly
ccd5e96A few other miscellaneous fixes to the doctests tests

comment:3 Changed 3 years ago by chapoton

does not apply

comment:4 Changed 3 years ago by chapoton

  • Dependencies #24313, #24234 deleted
  • Status changed from new to needs_info

comment:5 Changed 3 years ago by chapoton

  • Status changed from needs_info to needs_work

comment:6 Changed 3 years ago by embray

Working on rebasing this. This used to have all tests passing on Python 3, and still appears to there, but breaks some things on Python 2 now (I think it used to work there but now I'm not sure, it's been too long).

comment:7 Changed 3 years ago by git

  • Commit changed from ccd5e960503c867170f9bffd31364da35e962aca to 7dd7f9af21846d5d4d98815eff3cc409327dc793

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

aedc61aMiscellaneous fixes to tests in the doctest module.
1ee3e9bFix various minor encoding issues
96ae332Convert these classes to 'new-style' classes for consistency with Python 3
c040c67Use the restore_atexit context manager for this code to work on Python 3,
380a902Various fixes intended to ensure that opened files are closed explictly
92117e5A few other miscellaneous fixes to the doctests tests
72fdf00Python 2 will accept br as a string prefix but not rb
db544c7This exception reads 'utf-8' on Python 3 and 'utf8' on Python 2
7dd7f9aCorrect some encoding issues on Python 2.

comment:8 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

This has all the tests passing on Python 2, as well as on Python 3 except for a test that was added recently to FileDocTestSource.create_doctests (the "bitness" test). I'll worry about that later; it's not really a substantive failure.

comment:9 follow-up: Changed 3 years ago by jdemeyer

There is a lot of stuff happening in this branch. I would appreciate if this could be split up in smaller independent pieces, especially where you are making structural changes.

comment:10 follow-up: Changed 3 years ago by chapoton

When trying to use this branch in python3 to doctest a file, I get lots and lots of trivial doctests failures, typically like this one:

File "src/sage/combinat/yang_baxter_graph.py", line 576, in sage.combinat.yang_baxter_graph.YangBaxterGraph_partition.__init__
Failed example:
    Y = YangBaxterGraph(partition=[3,2,1]); Y
Expected:
    Yang-Baxter graph of [3, 2, 1], with top vertex (0, 1, 0, 2, 1, 0)
Got:
    b'Yang-Baxter graph of [3, 2, 1], with top vertex (0, 1, 0, 2, 1, 0)'

comment:11 in reply to: ↑ 10 Changed 3 years ago by embray

Replying to chapoton:

When trying to use this branch in python3 to doctest a file, I get lots and lots of trivial doctests failures, typically like this one:

File "src/sage/combinat/yang_baxter_graph.py", line 576, in sage.combinat.yang_baxter_graph.YangBaxterGraph_partition.__init__
Failed example:
    Y = YangBaxterGraph(partition=[3,2,1]); Y
Expected:
    Yang-Baxter graph of [3, 2, 1], with top vertex (0, 1, 0, 2, 1, 0)
Got:
    b'Yang-Baxter graph of [3, 2, 1], with top vertex (0, 1, 0, 2, 1, 0)'

That's likely. This is related to sage.repl.rich_ouput.backend_test. I have fixes to that coming in a separate ticket.

comment:12 in reply to: ↑ 9 ; follow-up: Changed 3 years ago by embray

Replying to jdemeyer:

There is a lot of stuff happening in this branch. I would appreciate if this could be split up in smaller independent pieces, especially where you are making structural changes.

You are welcome to look at the individual commits if that would be easier.

comment:13 Changed 3 years ago by embray

FWIW I opened this ticket two months ago and actually was using these fixes well before I opened the ticket to run the tests on Python 3, with consistent success (in that the test framework completes test runs and correctly reports passes and failures).

comment:14 in reply to: ↑ 12 Changed 3 years ago by jdemeyer

Replying to embray:

You are welcome to look at the individual commits if that would be easier.

The question is: am I supposed to review individual commits or the branch as a whole? I'm fine with doing either of those two extremes but rather not a mixture (I don't want to look at individual commits when you want me to review the branch as a whole).

comment:15 Changed 3 years ago by jdemeyer

...and if you really want me to review individual commits, I would prefer to split this over multiple tickets.

comment:16 follow-ups: Changed 3 years ago by embray

I'm not sure what the distinction is. There are literally thousands of things that need to be fixed for Python 3. Grouping up large sets of fixes on a per-module or per-package basis should get things done more quickly and with fewer merge problems, than if they were spread across multiple tickets of "miscellaneous fixes 1", "miscellaneous fixes 2", etc.

I'm pretty frustrated (and I believe Frédéric is too) that I've actually already fixed like 50% of the tests on Python 3 but have all the work squirreled away in my private branch, often resulting in duplicate effort (e.g. #24739). And the biggest bottleneck now is just the rate at which I can open logical tickets.

If you want to review some set of changes I don't see what difference it makes for you.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

Grouping up large sets of fixes on a per-module or per-package basis should get things done more quickly and with fewer merge problems, than if they were spread across multiple tickets of "miscellaneous fixes 1", "miscellaneous fixes 2", etc.

My opinion is the opposite. I find it easier to deal with many small isolated tickets instead of single large tickets. Mainly because it will allow easy things to pass immediately.

Speaking of frustration... I find your refusal to split up tickets frustrating. I'm not even asking you to split up the tickets. I'm asking for permission to split up your tickets.

comment:18 in reply to: ↑ 16 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

I'm not sure what the distinction is.

The distinction is mainly whether the commits are logically/functionally independent or not. If you have a first commit which is broken and then commit 2 and commit 3 to fix issues with the first commit, it makes no sense to review by commit. If on the other hand, every commit fixes a single issue, it does make sense to review by commit.

comment:19 in reply to: ↑ 17 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

Grouping up large sets of fixes on a per-module or per-package basis should get things done more quickly and with fewer merge problems, than if they were spread across multiple tickets of "miscellaneous fixes 1", "miscellaneous fixes 2", etc.

My opinion is the opposite. I find it easier to deal with many small isolated tickets instead of single large tickets. Mainly because it will allow easy things to pass immediately.

Speaking of frustration... I find your refusal to split up tickets frustrating. I'm not even asking you to split up the tickets. I'm asking for permission to split up your tickets.

Hey, if you're willing to do it be my guest. I don't see what you'd want to spend the time.

comment:20 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

I'm not sure what the distinction is.

The distinction is mainly whether the commits are logically/functionally independent or not. If you have a first commit which is broken and then commit 2 and commit 3 to fix issues with the first commit, it makes no sense to review by commit. If on the other hand, every commit fixes a single issue, it does make sense to review by commit.

That isn't really the case here. I believe there might be 1 or 2 fixup commits added late to this branch because it wasn't passing on Python 2, but those are actually worth keeping as separate commits since I felt they called out specific Python 2/3 differences that were worth making note of.

You should know by now that I know how to use rebase effectively and usually try to organize my commits in a logical fashion, and also that I frequently write detailed commit messages about what is being changed and why. I really don't understand why you care. If it were me I would just look at the changes--make note of any specific ones I want to comment on, and that's that.

Or just even glance over briefly to make sure nothing stands out, and then give you the benefit of the doubt. You don't have to scrutinize every single change everyone makes. Nobody has time for that.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

You don't have to scrutinize every single change everyone makes.

I almost feel bad to say that I am trying to do exactly that.

comment:22 in reply to: ↑ 21 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

You don't have to scrutinize every single change everyone makes.

I almost feel bad to say that I am trying to do exactly that.

And I appreciate that, because your scrutiny promotes a high level of quality. But I don't know how you then find time for much else. I'm a strong proponent of careful code review, but sometimes you also have to pick your battles, or trust other people to review things. I used to review every single line change to Astropy when we started the project, but as it grew there was no way I could keep up, especially as large modules grew in which different people had more expertise, and I learned to trust them to do their best in terms of quality assurance. I still give most changes a glance over in case there are specific issues I'm concerned about.

Anyways, you do what you want, and believe it or not I try my best to keep my changes reasonably atomic and logical (though this becomes difficult when making mass sets of minor fixes). And I'll do what I can to make things easier. I just wish I understood your process better and why you find it helpful to review a whole branch versus individual commits.

To me, looking over the full merge commit is obviously the first pass, and if there are individual changes I have more questions about I'll often drill more into the history of that file to find the individual commit where that change was made.

I agree this becomes more difficult if there are lots of commits combined that affect multiple files simultaneously. I wouldn't often combine changes like that into a single ticket (hence why I broke up #24740 and #24741, and considered actually breaking the latter into two more tickets). In this case, however, all the changes are fairly small an localized.

comment:23 follow-up: Changed 3 years ago by embray

To put things more constructively, if you can give me some idea how you prefer to see changes logically split into individual tickets, I can certainly keep that in mind in the future.

It's just that for the Python 3 fixes there are so many thousands of little changes, I thought it would be simplest (and least likely to produce conflicts) to group together large sets of small fixes by package, or in some cases by individual module.

Obviously if I require some more substantive change (such as new features) I can, and have, made separate tickets for that. Beyond that I'm not really sure.

comment:24 in reply to: ↑ 23 Changed 3 years ago by jdemeyer

Replying to embray:

To put things more constructively, if you can give me some idea how you prefer to see changes logically split into individual tickets, I can certainly keep that in mind in the future.

Broadly speaking, there are 3 level of changes:

  1. Small isolated changes and basic clean-up, e.g. adding str/bytes conversion, using with(open()), fixing imports, style changes.
  1. Non-trivial changes which do not change user-visible functionality, i.e. refactoring.
  1. Changes to user-visible functionality.

For me, a ticket should ideally work on precisely one of these levels. Level 1 changes can be mostly approved on sight. If a ticket consists of 80% level 1 changes and 20% level 2 changes, then it can be hard to understand the level 2 changes because the many level 1 changes are just distracting the diff.

large sets of small fixes

IMHO, this ticket contains several non-small (level 2 in my above classification) changes.

comment:25 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

comment:26 Changed 3 years ago by jdemeyer

  • Branch changed from u/embray/python3/sage-doctest to u/jdemeyer/python3/sage-doctest

comment:27 Changed 3 years ago by jdemeyer

  • Commit changed from 7dd7f9af21846d5d4d98815eff3cc409327dc793 to b16db63197e19e399656926a28ee58221d617020
  • Dependencies set to #24772
  • Status changed from needs_work to needs_review

New commits:

3eb2db6Miscellaneous fixes to tests in the doctest module.
51e20d9A few other miscellaneous fixes to the doctests tests
878a62bThis exception reads 'utf-8' on Python 3 and 'utf8' on Python 2
1ba0e67Fix various minor encoding issues
dcff893Correct some encoding issues on Python 2.
f513d1dA few other miscellaneous fixes to the doctests tests
bd5ddadVarious fixes intended to ensure that opened files are closed explictly
b16db63Use the restore_atexit context manager for this code to work on Python 3,

comment:28 Changed 3 years ago by git

  • Commit changed from b16db63197e19e399656926a28ee58221d617020 to ad6d069a1aed291531d75be70c980f3ccfa5e041

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

12ff3beFix various minor encoding issues
34876abCorrect some encoding issues on Python 2.
6110bc4Fix various minor encoding issues
c30c078A few other miscellaneous fixes to the doctests tests
9f5f429Various fixes intended to ensure that opened files are closed explictly
ad6d069Use the restore_atexit context manager for this code to work on Python 3,

comment:29 Changed 3 years ago by jdemeyer

Can you justify this change:

-        from six.moves.queue import Empty
-        try:
-            self.result = self.result_queue.get(block=False)
-        except Empty:
-            self.result = (0, DictAsObject(dict(err='noresult')))
-        del self.result_queue
-
-        self.outtmpfile.seek(0)
-        self.output = self.outtmpfile.read()
-        del self.outtmpfile
+        if self.result_queue is not None:
+            from six.moves.queue import Empty
+            try:
+                self.result = self.result_queue.get(block=False)
+            except Empty:
+                self.result = (0, DictAsObject(dict(err='noresult')))
+
+            self.result_queue = None
+
+        if self.outtmpfile is not None:
+            self.outtmpfile.seek(0)
+            self.output = bytes_to_str(self.outtmpfile.read())
+            self.outtmpfile.close()
+            self.outtmpfile = None

What was wrong with the old code?

comment:30 Changed 3 years ago by embray

I don't remember exactly, but I'm sure there was a good reason. Certainly I know that I didn't like that the old code was deleting attributes rather than setting them to None. I believe there were reasons (e.g. related to repeated calls of the same method) that it was saner to set these attributes to None when they were finished being used rather than deleting them.

comment:31 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

git history was rewritten in #24772, so this should be rebased.

comment:32 Changed 3 years ago by embray

Yeah, I was about to take care of that.

comment:33 follow-ups: Changed 3 years ago by embray

  • Branch changed from u/jdemeyer/python3/sage-doctest to u/embray/python3/sage-doctest
  • Commit changed from ad6d069a1aed291531d75be70c980f3ccfa5e041 to 47e464dda9aee8cb43651568ef721ee054cbab04
  • Status changed from needs_work to needs_review

Just took Jeroen's branch and rebased on top of #24772.

Is there anything more that still needs to be discussed on this?


New commits:

bf6cf97Correct some encoding issues on Python 2.
4b92b47A few other miscellaneous fixes to the doctests tests
ed24910Various fixes intended to ensure that opened files are closed explictly
47e464dUse the restore_atexit context manager for this code to work on Python 3,

comment:34 in reply to: ↑ 33 Changed 3 years ago by jdemeyer

Replying to embray:

Is there anything more that still needs to be discussed on this?

Probably yes. I haven't really looked in detail.

comment:35 Changed 3 years ago by embray

Probably? I'll note again that this has been working for me for months now. Maybe take a leap of faith so that we can get this in and others can finally start testing against Python 3.

comment:36 Changed 3 years ago by jdemeyer

As I said, I barely looked at this. And I prefer to do that after the dependencies have been merged.

comment:37 Changed 3 years ago by chapoton

  • Dependencies #24772 deleted

comment:38 in reply to: ↑ 33 Changed 3 years ago by jdemeyer

Replying to embray:

Just took Jeroen's branch and rebased on top of #24772.

The change that I mentioned in 29 is gone. Was that intentional? In any case, I would prefer not to add it back if there is no reason.

For ease of reviewing, I'm rebasing your branch on 8.2.beta7 (there are no conflicts, it's a trivial rebase). The 3 commits are independent and I did reorder them.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:39 Changed 3 years ago by jdemeyer

  • Branch changed from u/embray/python3/sage-doctest to u/jdemeyer/python3/sage-doctest

comment:40 Changed 3 years ago by git

  • Commit changed from 47e464dda9aee8cb43651568ef721ee054cbab04 to b4ca9e2144939f844d3a35788629b159278a8487

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bec1311Use the restore_atexit context manager for this code to work on Python 3,
e479f26Various fixes intended to ensure that opened files are closed explictly
b4ca9e2A few other miscellaneous fixes to the doctests tests

comment:41 Changed 3 years ago by jdemeyer

I'm moving the commit about atexit to #24922 because I want to add something.

comment:42 Changed 3 years ago by git

  • Commit changed from b4ca9e2144939f844d3a35788629b159278a8487 to 500a8a63da95d9709d30b83120f70ddc797c3ceb

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

d9c93a7Various fixes intended to ensure that opened files are closed explictly
500a8a6A few other miscellaneous fixes to the doctests tests

comment:43 Changed 3 years ago by embray

I didn't mean to delete it, and there was at least one good reason for it which is that deleting attributes off objects is just ugly when you can set them to None instead.

I'm reworking some of this code elsewhere so I'm not going to worry about it though.


New commits:

d9c93a7Various fixes intended to ensure that opened files are closed explictly
500a8a6A few other miscellaneous fixes to the doctests tests

comment:44 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This added call to save_result_output is causing breakage:

+        # Save any result output so far before we kill the process off
+        self.save_result_output()
sage -t --long src/sage/doctest/forker.py
**********************************************************************
File "src/sage/doctest/forker.py", line 2104, in sage.doctest.forker.DocTestWorker.save_result_output
Failed example:
    W.kill()
Exception raised:
    Traceback (most recent call last):
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 556, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 966, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.doctest.forker.DocTestWorker.save_result_output[14]>", line 1, in <module>
        W.kill()
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2176, in kill
        self.save_result_output()
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 2114, in save_result_output
        self.result = self.result_queue.get(block=False)
    AttributeError: 'DocTestWorker' object has no attribute 'result_queue'
**********************************************************************

Now I see that you needed 29 to fix that breakage but why did you need to add a call to save_result_output in kill()? All doctests pass when I remove that.

comment:46 Changed 3 years ago by jdemeyer

Does Python 3 no longer support the "a" mode in fdopen()? I consider that a bug. It might not matter for the doctesting framework, but the "w" and "a" options are not equivalent, especially when multiple processes write to the same file.

comment:47 follow-up: Changed 3 years ago by jdemeyer

Why are you wrapping all close() calls in a try/except block? If a file fails to close for some reason, that's a genuine error.

comment:48 follow-up: Changed 3 years ago by embray

I don't recall why the change from "w" to "a" mode. Maybe there was a reason, but in this case it seems I didn't document it. You you can try removing it and see. Maybe I was thinking about https://bugs.python.org/issue20082

comment:49 in reply to: ↑ 47 ; follow-up: Changed 3 years ago by embray

Replying to jdemeyer:

Why are you wrapping all close() calls in a try/except block? If a file fails to close for some reason, that's a genuine error.

Not "all". Those are in __del__ methods, where you can't assume the files are still even open.

comment:50 in reply to: ↑ 45 Changed 3 years ago by jdemeyer

Replying to embray:

https://git.sagemath.org/sage.git/commit/?id=2203ca27445e7db09cdf97b34896b6d8ea6eb670

I guess you mean this sentence from the commit message:

Note that DocTestWorker.kill() now calls sage_results_output [sic], which ensures that the DocTestWorker.outtmpfile is closed.

Why not simply call self.outtmpfile.close() in kill?

comment:51 in reply to: ↑ 49 Changed 3 years ago by jdemeyer

Replying to embray:

Not "all". Those are in __del__ methods, where you can't assume the files are still even open.

Even if the file is already closed, Python allows closing an already-closed file:

>>> f = open("/dev/null")
>>> f.close()
>>> f.close()

comment:52 in reply to: ↑ 48 ; follow-up: Changed 3 years ago by jdemeyer

Replying to embray:

I don't recall why the change from "w" to "a" mode. Maybe there was a reason, but in this case it seems I didn't document it. You you can try removing it and see. Maybe I was thinking about https://bugs.python.org/issue20082

No, it's worse than that:

Python 3.6.1 (default, Feb 19 2018, 09:57:52) 
[GCC 4.9.4] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.fdopen(2, "a")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/src/sage-config/local/lib/python3.6/os.py", line 1015, in fdopen
    return io.open(fd, *args, **kwargs)
OSError: [Errno 29] Illegal seek

comment:53 Changed 3 years ago by jdemeyer

(end of review for now)

comment:54 Changed 3 years ago by embray

Well, you could try removing it and see. I probably had a reason.

comment:55 in reply to: ↑ 52 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

I don't recall why the change from "w" to "a" mode. Maybe there was a reason, but in this case it seems I didn't document it. You you can try removing it and see. Maybe I was thinking about https://bugs.python.org/issue20082

No, it's worse than that:

Python 3.6.1 (default, Feb 19 2018, 09:57:52) 
[GCC 4.9.4] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.fdopen(2, "a")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/src/sage-config/local/lib/python3.6/os.py", line 1015, in fdopen
    return io.open(fd, *args, **kwargs)
OSError: [Errno 29] Illegal seek

Ah, right, this is https://bugs.python.org/issue27805

comment:56 Changed 3 years ago by embray

I still think whether it's needed or not, it's really ugly to just assign attributes to objects, and then delete them later, and have to do hasattr(obj, attr) on it when you could just set those attributes to None when they're not needed anymore. Maybe this is some deeply-ingrained Java-ism, but I think deleting attributes is in most cases an anti-pattern.

comment:57 Changed 3 years ago by embray

That said, I think maybe the kill() calls from 2203ca27 can be removed. I tried taking all that stuff and out I don't get any ResourceWarnings. So it's possible that while they did something at one time, additional fixes elsewhere rendered it unnecessary.

comment:58 Changed 3 years ago by embray

Same with the try/except around the close calls. I don't seem to have made note of a specific reason for them.

comment:59 Changed 3 years ago by embray

  • Branch changed from u/jdemeyer/python3/sage-doctest to u/embray/python3/sage-doctest
  • Commit changed from 500a8a63da95d9709d30b83120f70ddc797c3ceb to ba9f9e771b56e7a2b19ef24ee481b8413d3843fc
  • Status changed from needs_work to needs_review

Okay, I removed a few of the changes you pointed out that don't appear to be necessary. Other than that I wouldn't mess with anything else.


New commits:

87a56deVarious fixes intended to ensure that opened files are closed explictly where
ba9f9e7A few other miscellaneous fixes to the doctests tests

comment:60 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

Hold on, I thought I removed all those try/excepts...

comment:61 Changed 3 years ago by git

  • Commit changed from ba9f9e771b56e7a2b19ef24ee481b8413d3843fc to 436ed266f954ecf03f894b9c6539cca353a68ad6

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

af86e5cVarious fixes intended to ensure that opened files are closed explictly where
436ed26A few other miscellaneous fixes to the doctests tests

comment:62 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

Okay.

comment:63 Changed 3 years ago by jdemeyer

OK, that seems good on first sight. The last commit (changing the sleep() time) is probably no longer needed. I guess you added it because kill() did more stuff and needed more time.

And let me just think whether the change of open() mode from "a" to "w" could cause trouble...

comment:64 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:65 Changed 3 years ago by git

  • Commit changed from 436ed266f954ecf03f894b9c6539cca353a68ad6 to 2155133fa6f901ac92ea0ba46ee5ea403ee4f18f
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

2155133missing string encoding fix

comment:66 Changed 3 years ago by embray

Sorry, hopefully not too late to append to this ticket. This little fix should have been here, and I think got lost somewhere along the way.

comment:67 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

comment:68 Changed 3 years ago by vbraun

  • Branch changed from u/embray/python3/sage-doctest to 2155133fa6f901ac92ea0ba46ee5ea403ee4f18f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:69 Changed 3 years ago by embray

  • Commit 2155133fa6f901ac92ea0ba46ee5ea403ee4f18f deleted

I think some of the stuff I ended up removing from this was needed after all. It might not have shown up either because:

a) I was originally testing with a debug build of Python which always displays ResourceWarnings by default (a non-debug build does not)

b) Some of it might have been particular to Cygwin, but I switched mostly to testing on Linux later on.

Note: See TracTickets for help on using tickets.