Repository: twisted/twisted

Issue:
`trial -j` silently hangs if a logfile with a directory separator in its name is passed
Make a test file like this:

```python3
from twisted.trial.unittest import TestCase

class NothingTest(TestCase):
    def test_nothing(self) -> None:
        "Do nothing."
```

Then,

```
$ mkdir hello
$ trial -j 1 -l hello/test.log ./test_nothing.py
Running 1 tests.
^CTraceback (most recent call last):
  File "/Users/glyph/.virtualenvs/Twisted/bin/trial", line 33, in <module>
    sys.exit(load_entry_point('Twisted', 'console_scripts', 'trial')())
  File "/Users/glyph/Projects/Twisted/src/twisted/scripts/trial.py", line 654, in run
    test_result = trialRunner.run(suite)
  File "/Users/glyph/Projects/Twisted/src/twisted/trial/_dist/disttrial.py", line 453, in run
    result.raiseException()
  File "/Users/glyph/Projects/Twisted/src/twisted/python/failure.py", line 503, in raiseException
    raise self.value.with_traceback(self.tb)
twisted.python.filepath.InsecurePath: 'hello/test.log' contains one or more directory separators
Unhandled error in Deferred:

Traceback (most recent call last):
  File "/Users/glyph/Projects/Twisted/src/twisted/scripts/trial.py", line 654, in run
    test_result = trialRunner.run(suite)
  File "/Users/glyph/Projects/Twisted/src/twisted/trial/_dist/disttrial.py", line 449, in run
    d.addBoth(lambda ignored: self._reactor.stop())
  File "/Users/glyph/Projects/Twisted/src/twisted/internet/defer.py", line 572, in addBoth
    return self.addCallbacks(
  File "/Users/glyph/Projects/Twisted/src/twisted/internet/defer.py", line 511, in addCallbacks
    self._runCallbacks()
--- <exception caught here> ---
  File "/Users/glyph/Projects/Twisted/src/twisted/internet/defer.py", line 891, in _runCallbacks
    current.result = callback(  # type: ignore[misc]
  File "/Users/glyph/Projects/Twisted/src/twisted/trial/_dist/disttrial.py", line 449, in <lambda>
    d.addBoth(lambda ignored: self._reactor.stop())
  File "/Users/glyph/Projects/Twisted/src/twisted/internet/base.py", line 708, in stop
    raise error.ReactorNotRunning("Can't stop reactor that isn't running.")
twisted.internet.error.ReactorNotRunning: Can't stop reactor that isn't running.
$ 
```

You'll see the `^C` there, because `trial` just hangs forever.

Discussion:

Patch:
diff --git a/src/twisted/trial/_dist/disttrial.py b/src/twisted/trial/_dist/disttrial.py
index 11b645d933..9d831c0c08 100644
--- a/src/twisted/trial/_dist/disttrial.py
+++ b/src/twisted/trial/_dist/disttrial.py
@@ -12,6 +12,7 @@ responsible for coordinating all of trial's behavior at the highest level.
 import os
 import sys
 from functools import partial
+from os.path import isabs
 from typing import Awaitable, Callable, Iterable, List, Sequence, TextIO, Union, cast
 from unittest import TestResult, TestSuite
 
@@ -212,9 +213,15 @@ class WorkerPool:
             self._config.workingDirectory,
         )
 
-        # Open a log file in the chosen working directory (not necessarily the
-        # same as our configured working directory, if that path was in use).
-        testLog = openTestLog(testDir.child(self._config.logFile))
+        if isabs(self._config.logFile):
+            # Open a log file wherever the user asked.
+            testLogPath = FilePath(self._config.logFile)
+        else:
+            # Open a log file in the chosen working directory (not necessarily
+            # the same as our configured working directory, if that path was
+            # in use).
+            testLogPath = testDir.preauthChild(self._config.logFile)
+        testLog = openTestLog(testLogPath)
 
         ampWorkers = [LocalWorkerAMP() for x in range(self._config.numWorkers)]
         workers = self._createLocalWorkers(
diff --git a/src/twisted/trial/newsfragments/11580.bugfix b/src/twisted/trial/newsfragments/11580.bugfix
new file mode 100644
index 0000000000..c20b098b80
--- /dev/null
+++ b/src/twisted/trial/newsfragments/11580.bugfix
@@ -0,0 +1,1 @@
+`trial -jN --logfile path` no longer hangs if *path* contains a directory separator.
\ No newline at end of file
diff --git a/src/twisted/trial/newsfragments/11657.feature b/src/twisted/trial/newsfragments/11657.feature
new file mode 100644
index 0000000000..7d3136ff23
--- /dev/null
+++ b/src/twisted/trial/newsfragments/11657.feature
@@ -0,0 +1,1 @@
+twisted.trial.unittest.SynchronousTestCase.successResultOf is now annotated as accepting coroutines.
diff --git a/src/twisted/trial/runner.py b/src/twisted/trial/runner.py
index 186959536f..47167718b4 100644
--- a/src/twisted/trial/runner.py
+++ b/src/twisted/trial/runner.py
@@ -837,7 +837,7 @@ class TrialRunner:
             self._logFileObject.close()
             self._logFileObject = None
 
-    def _setUpLogFile(self):
+    def _setUpLogFile(self) -> None:
         self._tearDownLogFile()
         if self.logfile == "-":
             logFile = sys.stdout
diff --git a/src/twisted/trial/util.py b/src/twisted/trial/util.py
index a5840c00de..3f064801ea 100644
--- a/src/twisted/trial/util.py
+++ b/src/twisted/trial/util.py
@@ -329,6 +329,7 @@ def _unusedTestDirectory(base):
         else:
             testdir = base
 
+        testdir.parent().makedirs(ignoreExistingDirectory=True)
         testDirLock = FilesystemLock(testdir.path + ".lock")
         if testDirLock.lock():
             # It is not in use
@@ -393,6 +394,7 @@ def openTestLog(path: FilePath) -> TextIO:
     """
     Open the given path such that test log messages can be written to it.
     """
+    path.parent().makedirs(ignoreExistingDirectory=True)
     # Always use UTF-8 because, considering all platforms, the system default
     # encoding can not reliably encode all code points.
     return open(path.path, "a", encoding="utf-8", errors="strict")


Test patch:
diff --git a/src/twisted/trial/_dist/test/test_disttrial.py b/src/twisted/trial/_dist/test/test_disttrial.py
index 5114014f03..f15333c32a 100644
--- a/src/twisted/trial/_dist/test/test_disttrial.py
+++ b/src/twisted/trial/_dist/test/test_disttrial.py
@@ -9,12 +9,24 @@ import os
 import sys
 from functools import partial
 from io import StringIO
+from os.path import sep
 from typing import Callable, List, Set
 
 from zope.interface import implementer, verify
 
-from attrs import Factory, define, field
-from hamcrest import assert_that, contains, equal_to, has_length, none
+from attrs import Factory, assoc, define, field
+from hamcrest import (
+    assert_that,
+    contains,
+    ends_with,
+    equal_to,
+    has_length,
+    none,
+    starts_with,
+)
+from hamcrest.core.core.allof import AllOf
+from hypothesis import given
+from hypothesis.strategies import booleans, sampled_from
 
 from twisted.internet import interfaces
 from twisted.internet.base import ReactorBase
@@ -304,6 +316,40 @@ class WorkerPoolTests(TestCase):
         assert_that(started.testLog.closed, equal_to(True))
         assert_that(started.testDirLock.locked, equal_to(False))
 
+    @given(
+        booleans(),
+        sampled_from(
+            [
+                "out.log",
+                f"subdir{sep}out.log",
+            ]
+        ),
+    )
+    def test_logFile(self, absolute: bool, logFile: str) -> None:
+        """
+        L{WorkerPool.start} creates a L{StartedWorkerPool} configured with a
+        log file based on the L{WorkerPoolConfig.logFile}.
+        """
+        if absolute:
+            logFile = self.parent.path + sep + logFile
+
+        config = assoc(self.config, logFile=logFile)
+
+        if absolute:
+            matches = equal_to(logFile)
+        else:
+            matches = AllOf(
+                # This might have a suffix if the configured workingDirectory
+                # was found to be in-use already so we don't add a sep suffix.
+                starts_with(config.workingDirectory.path),
+                # This should be exactly the suffix so we add a sep prefix.
+                ends_with(sep + logFile),
+            )
+
+        pool = WorkerPool(config)
+        started = self.successResultOf(pool.start(CountingReactor([])))
+        assert_that(started.testLog.name, matches)
+
 
 class DistTrialRunnerTests(TestCase):
     """
diff --git a/src/twisted/trial/_synctest.py b/src/twisted/trial/_synctest.py
index dcd091f725..694e284753 100644
--- a/src/twisted/trial/_synctest.py
+++ b/src/twisted/trial/_synctest.py
@@ -17,7 +17,19 @@ import types
 import unittest as pyunit
 import warnings
 from dis import findlinestarts as _findlinestarts
-from typing import Iterable, List, NoReturn, Optional, Tuple, Type, TypeVar, Union
+from typing import (
+    Any,
+    Coroutine,
+    Generator,
+    Iterable,
+    List,
+    NoReturn,
+    Optional,
+    Tuple,
+    Type,
+    TypeVar,
+    Union,
+)
 
 # Python 2.7 and higher has skip support built-in
 from unittest import SkipTest
@@ -669,18 +681,30 @@ class _Assertions(pyunit.TestCase):
 
     failIfIsInstance = assertNotIsInstance
 
-    def successResultOf(self, deferred: Deferred[T]) -> T:
+    def successResultOf(
+        self,
+        deferred: Union[
+            Coroutine[Deferred[T], Any, T],
+            Generator[Deferred[T], Any, T],
+            Deferred[T],
+        ],
+    ) -> T:
         """
         Return the current success result of C{deferred} or raise
         C{self.failureException}.
 
-        @param deferred: A L{Deferred<twisted.internet.defer.Deferred>} which
-            has a success result.  This means
+        @param deferred: A L{Deferred<twisted.internet.defer.Deferred>} or
+            I{coroutine} which has a success result.
+
+            For a L{Deferred<twisted.internet.defer.Deferred>} this means
             L{Deferred.callback<twisted.internet.defer.Deferred.callback>} or
             L{Deferred.errback<twisted.internet.defer.Deferred.errback>} has
             been called on it and it has reached the end of its callback chain
-            and the last callback or errback returned a non-L{failure.Failure}.
-        @type deferred: L{Deferred<twisted.internet.defer.Deferred>}
+            and the last callback or errback returned a
+            non-L{failure.Failure}.
+
+            For a I{coroutine} this means all awaited values have a success
+            result.
 
         @raise SynchronousTestCase.failureException: If the
             L{Deferred<twisted.internet.defer.Deferred>} has no result or has a
diff --git a/src/twisted/trial/test/matchers.py b/src/twisted/trial/test/matchers.py
index 1928254886..5da818ff6d 100644
--- a/src/twisted/trial/test/matchers.py
+++ b/src/twisted/trial/test/matchers.py
@@ -5,12 +5,15 @@
 Hamcrest matchers useful throughout the test suite.
 """
 
-from typing import Callable, TypeVar
+from typing import IO, Callable, Optional, TypeVar
 
 from hamcrest.core.base_matcher import BaseMatcher
 from hamcrest.core.description import Description
 from hamcrest.core.matcher import Matcher
 
+from twisted.python.filepath import IFilePath
+from twisted.python.reflect import fullyQualifiedName
+
 _A = TypeVar("_A")
 _B = TypeVar("_B")
 
@@ -21,17 +24,47 @@ class _MatchAfter(BaseMatcher[_A]):
 
     @ivar f: The function to apply.
     @ivar m: The matcher to use on the result.
+
+    @ivar _e: After trying to apply the function fails with an exception, the
+        exception that was raised.  This can later be used by
+        L{describe_mismatch}.
     """
 
     def __init__(self, f: Callable[[_A], _B], m: Matcher[_B]):
         self.f = f
         self.m = m
+        self._e: Optional[Exception] = None
 
     def _matches(self, item: _A) -> bool:
         """
         Apply the function and delegate matching on the result.
         """
-        return self.m.matches(self.f(item))
+        try:
+            transformed = self.f(item)
+        except Exception as e:
+            self._e = e
+            return False
+        else:
+            return self.m.matches(transformed)
+
+    def describe_mismatch(self, item: _A, mismatch_description: Description) -> None:
+        """
+        Describe the mismatching item or the exception that occurred while
+        pre-processing it.
+
+        @note: Since the exception reporting here depends on mutable state it
+            will only work as long as PyHamcrest calls methods in the right
+            order.  The PyHamcrest Matcher interface doesn't seem to allow
+            implementing this functionality in a more reliable way (see the
+            implementation of L{assert_that}).
+        """
+        if self._e is None:
+            super().describe_mismatch(item, mismatch_description)
+        else:
+            mismatch_description.append_text(
+                f"{fullyQualifiedName(self.f)}({item!r}) raised\n"
+                f"{fullyQualifiedName(self._e.__class__)}: {self._e}"
+            )
 
     def describe_to(self, description: Description) -> None:
         """
@@ -46,3 +79,17 @@ def after(f: Callable[[_A], _B], m: Matcher[_B]) -> Matcher[_A]:
     Create a matcher which calls C{f} and uses C{m} to match the result.
     """
     return _MatchAfter(f, m)
+
+
+def fileContents(m: Matcher[str], encoding: str = "utf-8") -> Matcher[IFilePath]:
+    """
+    Create a matcher which matches a L{FilePath} the contents of which are
+    matched by L{m}.
+    """
+
+    def getContent(p: IFilePath) -> str:
+        f: IO[bytes]
+        with p.open() as f:
+            return f.read().decode(encoding)
+
+    return after(getContent, m)
diff --git a/src/twisted/trial/test/test_matchers.py b/src/twisted/trial/test/test_matchers.py
new file mode 100644
index 0000000000..f6fb1aa7bb
--- /dev/null
+++ b/src/twisted/trial/test/test_matchers.py
@@ -0,0 +1,87 @@
+"""
+Tests for L{twisted.trial.test.matchers}.
+"""
+from hamcrest import anything, assert_that, contains_string, equal_to, not_
+from hamcrest.core.core.allof import AllOf
+from hamcrest.core.string_description import StringDescription
+from hypothesis import given
+from hypothesis.strategies import just, sampled_from, text
+
+from twisted.python.filepath import FilePath
+from twisted.trial.unittest import SynchronousTestCase
+from .matchers import fileContents
+
+
+class FileContentsTests(SynchronousTestCase):
+    """
+    Tests for L{fileContents}.
+    """
+
+    @given(text(), just("utf-8"))
+    def test_matches(self, contents: str, encoding: str) -> None:
+        """
+        L{fileContents} matches a L{IFilePath} that refers to a file that
+        contains a string that is matched by the parameterized matcher.
+
+        :param contents: The text string to place in the file and match
+            against.
+
+        :param encoding: The text encoding to use to encode C{contents} when
+            writing to the file.
+        """
+        p = FilePath(self.mktemp())
+        p.setContent(contents.encode(encoding))
+
+        description = StringDescription()
+        assert_that(
+            fileContents(equal_to(contents)).matches(p, description), equal_to(True)
+        )
+        assert_that(str(description), equal_to(""))
+
+    @given(
+        just("some text, it doesn't matter what"),
+        sampled_from(["ascii", "latin-1", "utf-8"]),
+    )
+    def test_mismatches(self, contents: str, encoding: str) -> None:
+        """
+        L{fileContents} does not match an L{IFilePath} that refers to a
+        file that contains a string that is not matched by the parameterized
+        matcher.
+
+        :param contents: The text string to place in the file and match
+            against.
+
+        :param encoding: The text encoding to use to encode C{contents} when
+            writing to the file.
+        """
+        p = FilePath(self.mktemp())
+        p.setContent(contents.encode(encoding))
+
+        description = StringDescription()
+        assert_that(
+            fileContents(not_(anything())).matches(p, description), equal_to(False)
+        )
+        assert_that(str(description), equal_to(f"was <{p}>"))
+
+    def test_ioerror(self):
+        """
+        L{fileContents} reports details of any I/O error encountered while
+        attempting to match.
+        """
+        p = FilePath(self.mktemp())
+
+        description = StringDescription()
+        assert_that(fileContents(anything()).matches(p, description), equal_to(False))
+        assert_that(
+            str(description),
+            # It must contain at least ...
+            AllOf(
+                # the name of the matcher.
+                contains_string("fileContents"),
+                # the name of the exception raised.
+                contains_string("FileNotFoundError"),
+                # the repr (so weird values are escaped) of the path being
+                # matched against.
+                contains_string(repr(p.path)),
+            ),
+        )
diff --git a/src/twisted/trial/test/test_script.py b/src/twisted/trial/test/test_script.py
index abc1d3fefd..b12fcd5695 100644
--- a/src/twisted/trial/test/test_script.py
+++ b/src/twisted/trial/test/test_script.py
@@ -8,13 +8,20 @@ import sys
 import textwrap
 import types
 from io import StringIO
+from typing import List
 
+from hamcrest import assert_that, contains_string
+from hypothesis import given
+from hypothesis.strategies import sampled_from
+
+from twisted.logger import Logger
 from twisted.python import util
 from twisted.python.filepath import FilePath
 from twisted.python.usage import UsageError
 from twisted.scripts import trial
 from twisted.trial import unittest
 from twisted.trial._dist.disttrial import DistTrialRunner
+from twisted.trial._dist.functional import compose
 from twisted.trial.runner import (
     DestructiveTestSuite,
     TestLoader,
@@ -22,6 +29,7 @@ from twisted.trial.runner import (
     TrialRunner,
 )
 from twisted.trial.test.test_loader import testNames
+from .matchers import fileContents
 
 pyunit = __import__("unittest")
 
@@ -33,6 +41,99 @@ def sibpath(filename):
     return util.sibpath(__file__, filename)
 
 
+def logSomething() -> None:
+    """
+    Emit something to L{twisted.logger}.
+    """
+    Logger().info("something")
+
+
+def parseArguments(argv: List[str]) -> trial.Options:
+    """
+    Parse an argument list using trial's argument parser.
+    """
+    config = trial.Options()
+    config.parseOptions(argv)
+    return config
+
+
+def runFromConfig(config: trial.Options) -> trial.Options:
+    """
+    Run L{logSomething} as a test method using the given configuration.
+    """
+    runner = trial._makeRunner(config)
+    runner.stream = StringIO()
+    suite = TestSuite([pyunit.FunctionTestCase(logSomething)])
+    runner.run(suite)
+    return config
+
+
+runFromArguments = compose(runFromConfig, parseArguments)
+
+
+class LogfileTests(unittest.SynchronousTestCase):
+    """
+    Tests for the --logfile option.
+    """
+
+    @given(
+        sampled_from(
+            [
+                "dir-a",
+                "dir-b",
+                "dir-c/dir-d",
+            ]
+        )
+    )
+    def test_default(self, workingDirectory: str) -> None:
+        """
+        If no value is given for the option then logs are written to a log
+        file constructed from a default value.
+        """
+        config = runFromArguments(["--temp-directory", workingDirectory])
+        logPath = FilePath(workingDirectory).preauthChild(config["logfile"])
+        assert_that(logPath, fileContents(contains_string("something")))
+
+    @given(
+        sampled_from(
+            [
+                "somelog.txt",
+                "somedir/somelog.txt",
+            ]
+        )
+    )
+    def test_relativePath(self, logfile: str) -> None:
+        """
+        If the value given for the option is a relative path then it is
+        interpreted relative to trial's own temporary working directory and
+        logs are written there.
+        """
+        config = runFromArguments(["--logfile", logfile])
+        logPath = FilePath(config["temp-directory"]).preauthChild(logfile)
+        assert_that(logPath, fileContents(contains_string("something")))
+
+    @given(
+        sampled_from(
+            [
+                "somelog.txt",
+                "somedir/somelog.txt",
+            ]
+        )
+    )
+    def test_absolutePath(self, logfile: str) -> None:
+        """
+        If the value given for the option is an absolute path then it is
+        interpreted absolutely and logs are written there.
+        """
+        # We don't want to scribble to arbitrary places on the host
+        # filesystem.  Construct an absolute path that's beneath our working
+        # directory - which trial will make into a per-run unique temporary
+        # directory.
+        logPath = FilePath(".").preauthChild(logfile)
+        runFromArguments(["--logfile", logPath.path])
+        assert_that(logPath, fileContents(contains_string("something")))
+
+
 class ForceGarbageCollectionTests(unittest.SynchronousTestCase):
     """
     Tests for the --force-gc option.