test_no_spawn_no_stdin_attached tests the build environment

[kpcyrd]

hello, just letting you know alot currently fails in the Arch Linux reproducible builds system.

Since the original build passed it seems the test heavily depends on the terminal attached to the build, which it shouldn't.

test_char_special (tests.utils.test_argparse.TestRequireFile) ... ok
test_dir (tests.utils.test_argparse.TestRequireFile) ... ok
test_doesnt_exist (tests.utils.test_argparse.TestRequireFile) ... ok
test_fifo (tests.utils.test_argparse.TestRequireFile) ... ok
test_file (tests.utils.test_argparse.TestRequireFile) ... ok
test_validates (tests.utils.test_argparse.TestValidatedStore) ... ok
test_empty_strings_are_converted_to_empty_lists (tests.utils.test_configobj.TestForceList) ... ok
test_strings_are_converted_to_single_item_lists (tests.utils.test_configobj.TestForceList) ... ok
test_hash_for_unicode_representation (tests.widgets.test_globals.TestTagWidget) ... ok
test_sort (tests.widgets.test_globals.TestTagWidget)
Test sorting. ... ok

======================================================================
FAIL: test_no_spawn_no_stdin_attached (tests.commands.test_global.TestExternalCommand)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/alot/src/alot-0.9.1/tests/utilities.py", line 189, in _actual
    return loop.run_until_complete(coro(*args, **kwargs))
  File "/usr/lib/python3.9/asyncio/base_events.py", line 642, in run_until_complete
    return future.result()
  File "/build/alot/src/alot-0.9.1/tests/commands/test_global.py", line 129, in test_no_spawn_no_stdin_attached
    ui.notify.assert_not_called()
  File "/usr/lib/python3.9/unittest/mock.py", line 868, in assert_not_called
    raise AssertionError(msg)
AssertionError: Expected 'notify' to not have been called. Called 1 times.
Calls: [call('editor has exited with error code 1 -- No stderr output', priority='error')].

----------------------------------------------------------------------
Ran 345 tests in 3.432s

FAILED (failures=1, expected failures=9)
Test failed: <unittest.runner.TextTestResult run=345 errors=0 failures=1>
error: Test failed: <unittest.runner.TextTestResult run=345 errors=0 failures=1>
==> ERROR: A failure occurred in check().
    Aborting...
  -> Delete snapshot for alot_3270817...

[pazz]

Thanks for reporting. Is this a broken test or alot's functionality malforming? Either way, a PR would be welcome. Cheers, P

[kpcyrd]

I think it's only the test, it only affects some build environments and the application itself is probably fine.

[lucc]

The test code is this:

    @utilities.async_test
    async def test_no_spawn_no_stdin_attached(self):
        ui = utilities.make_ui()
        cmd = g_commands.ExternalCommand('test -t 0', refocus=False)
        await cmd.apply(ui)
        ui.notify.assert_not_called()

The ExternalCommand class is just mainly a wrapper around asyncio.create_subprocess_{exec,shell} and here we run test -t 0. In the next test (which succeeded) we run test -t 0 while sending some text to it from python and then expect the command to fail.

@kpcyrd I think "it seems the test heavily depends on the terminal attached to the build" is a little to strong, my conclusion would be "the test depends an any terminal being attached". And then my question is: Is there a terminal attached to the build? What kind of infrastructure are you using to run these tests? Do I assume correctly that test -t 0 in a shell would fail in that infrastructure?

And the question @pazz and @dcbaker is: This test was introduced in 5f96f0d , do we really need a terminal or can we test this some other way? What exactly are we trying to test?

[dcbaker]

That set of tests is fixed in the next commit, which seems to be for #1137, so what's supposed to be tested is that the subprocess.Popen call is invoked with it's stdin parameter populated correctly.

[lucc]

The same test was troubling me when running tests in github ci: https://github.com/pazz/alot/pull/1617/commits/7bf4dee4ea7288e0392aa6c9e2053062fcecffb5

[kpcyrd]

Sorry for not following up for 2 years, the infrastructure this is running on is https://reproducible.archlinux.org/, the chain of execution is rebuilderd -> archlinux-repro -> makepkg -> alot tests, archlinux-repro and makepkg just pass through stdin/stdout/stderr, but rebuilderd explicitly sets /dev/null for stdin and a pipe for stdout and stderr to capture the child processes output:

https://github.com/kpcyrd/rebuilderd/blob/0fda367320ef18b65dcf6890c168b67a9cff65df/worker/src/proc.rs#L122-L146

This is done so the website can show the build output for all the packages we build.

[lucc]

We have a nix flake in this repository and nix also sets stdin to /dev/null in the build sandbox. I just verified this and inside the build sandbox of nix I get

lrwxrwxrwx 1 nobody nogroup 15 Dec  8 14:44 /dev/stderr -> /proc/self/fd/2
lrwxrwxrwx 1 nobody nogroup 15 Dec  8 14:44 /dev/stdin -> /proc/self/fd/0
lrwxrwxrwx 1 nobody nogroup 15 Dec  8 14:44 /dev/stdout -> /proc/self/fd/1
lrwx------ 1 nixbld nixbld  64 Dec  8 14:44 /proc/self/fd/0 -> /dev/null
lrwx------ 1 nixbld nixbld  64 Dec  8 14:44 /proc/self/fd/1 -> /dev/pts/2
lrwx------ 1 nixbld nixbld  64 Dec  8 14:44 /proc/self/fd/2 -> /dev/pts/2

So if the test succeeds in the nix sandbox it should also succeed in the arch sandbox.

[pazz]

@lucc about your question above, I 'd be surprised if any of our tests actually require a terminal. Do you think we need to adjust/remove that test?

[lucc]

@pazz I conclude from @dcbaker's comment that we do not need to test for an actual terminal device to be attached. We rather want to know if we are successfully redirecting text into stdin or if we pass stdin=None to subprocess.Popen (which according to the docs means "With the default settings of None, no redirection will occur.")

We could either find a shell command that gives the correct exit status in all environments where we want to run our tests (terminal, github, nix, arch build, etc). For this I played around in a terminal for a bit and came up with this.

$ t () { 
> test -t 0; echo "test -t 0 ==> $?"
> test -p /dev/stdin; echo "test -p /dev/stdin ==> $?"
> test -c /dev/stdin; echo "test -c /dev/stdin ==> $?"
> }
$ t
test -t 0 ==> 0
test -p /dev/stdin ==> 1
test -c /dev/stdin ==> 0
$ echo foo | t
test -t 0 ==> 1
test -p /dev/stdin ==> 0
test -c /dev/stdin ==> 1
$ cat /dev/null | t
test -t 0 ==> 1
test -p /dev/stdin ==> 0
test -c /dev/stdin ==> 1
$ t < /dev/null
test -t 0 ==> 1
test -p /dev/stdin ==> 1
test -c /dev/stdin ==> 0

Maybe test -p /dev/stdin is a suitable candidate.

Alternatively we can rewrite the test to check if Popen did some redirection of stdin. We can get the full path of stdin of our python test code with os.path.realpath("/dev/stdin") and compare that the the result of the shell command realpath /dev/stdin. That adds a dependency on the shell command realpath but that is included in coreutils and busybox so maybe we are good. I suspect @dcbaker chose test in order not to introduce any dependencies.

[lucc]

For me this change makes the tests succeed in the terminal and in the nix sandbox. I am running Linux so maybe that is different on Mac or BSD. Can somebody confirm?

diff --git a/flake.nix b/flake.nix
index 339f6587..3cd0609a 100644
--- a/flake.nix
+++ b/flake.nix
@@ -36,10 +36,6 @@

             nativeCheckInputs = with pkgs; [ gnupg notmuch procps ];
             checkPhase = ''
-              # In the nix sandbox stdin is not a terminal but /dev/null so we
-              # change the shell command only in this specific test.
-              sed -i '/test_no_spawn_no_stdin_attached/,/^$/s/test -t 0/sh -c "[ $(wc -l) -eq 0 ]"/' tests/commands/test_global.py
-
               python3 -m unittest -v
             '';
           });
diff --git a/tests/commands/test_global.py b/tests/commands/test_global.py
index 775a822c..8932f77a 100644
--- a/tests/commands/test_global.py
+++ b/tests/commands/test_global.py
@@ -124,19 +124,19 @@ class TestExternalCommand(unittest.TestCase):
     @utilities.async_test
     async def test_no_spawn_no_stdin_attached(self):
         ui = utilities.make_ui()
-        cmd = g_commands.ExternalCommand('test -t 0', refocus=False)
+        cmd = g_commands.ExternalCommand('test -p /dev/stdin', refocus=False)
         await cmd.apply(ui)
-        ui.notify.assert_not_called()
+        ui.notify.assert_called_once_with(
+                'editor has exited with error code 1 -- No stderr output',
+                priority='error')

     @utilities.async_test
     async def test_no_spawn_stdin_attached(self):
         ui = utilities.make_ui()
         cmd = g_commands.ExternalCommand(
-            "test -t 0", stdin='0', refocus=False)
+            "test -p /dev/stdin", stdin='0', refocus=False)
         await cmd.apply(ui)
-        ui.notify.assert_called_once_with(
-                'editor has exited with error code 1 -- No stderr output',
-                priority='error')
+        ui.notify.assert_not_called()

     @utilities.async_test
     async def test_no_spawn_failure(self):