[PATCH] Fish: fix perl ls helper

[and]

With #3599 I introduce a perl warning in fish_list_perl()
so fish_list_perl() was skipped (return code 255) and fallback ls
function was used instead.

Plus all % chars must quoted because of g_strconcat() after reading
script file into string.

[zaytsev]

Nah, I should have found some way to test the Perl helper as well, I only actually tested the ls part, which worked :-/

[zaytsev]

@and, since you broke it in #3599, do you think that in addition to providing a patch, you could cover the FISH scripts with unit tests as a punishment?

This shouldn't be too complicated (specifically, see chapters about Simple Tests), but this will make for the first time possible to change the venerable FISH scripts without panic fear:

​https://www.gnu.org/software/automake/manual/html_node/Tests.html

Right now, if you change anything there you have to do rigorous manual testing with different hosts and pray that nothing has got broken by the new changes...

[zaytsev]

(In #3613) Closed as duplicate of #3611.

[zaytsev]

Ticket #3613 has been marked as a duplicate of this ticket.

[zaytsev-work]

Also see comments in #3620.

[mooffie]

Replying to zaytsev:

@and, since you broke it in #3599, do you think that in addition to providing a patch, you could cover the FISH scripts with unit tests as a punishment?

BTW, do you know of a public SSH server where FISH tests could be run against? (I googled a bit but couldn't find any).

Now, I know mc² is off-topic, but I want to mention that it's fun ​to write tests in it. For example, to test FISH:

local ensure = devel.ensure

local function test_fish()

  -- ticket #3599.
  local stat = assert(fs.stat('/sh://mooffie@mooffie.typo.co.il/dev/tty1'))
  ensure.equal(stat.rdev, 0x401,
    'The filesystem reports major:minor correctly for device files.')

  -- ticket #3611.
  ensure.equal(tglob('/sh://mooffie@mooffie.typo.co.il/etc/fs[t]ab'), {'/etc/fstab'},
    'The filesystem reports its files.') 

end

(Manual pages: ​fs, ​devel.ensure)

There's already a ​test that covers many file operations. Right now it does its stuff in '/tmp', but by changing this to '/​sh://whatever-server.you-want/tmp' you'd get a complete coverage of FISH.

As another example, here's a trivial test for the subshell:

local function test_subshell()

  fs.chdir('/')
  mc.execute('cd /usr/bin')
  ensure.equal(fs.current_dir(), '/usr/bin',
    'The subshell works and reports back its directory.')

end

(Manual entries: ​mc.execute)

[zaytsev-work]

BTW, do you know of a public SSH server where FISH tests could be run against? (I googled a bit but couldn't find any).

To be honest, I don't know of any; of course, we could set up an account on our development VM, but that doesn't sound like a good solution to me at any rate.

However, I don't think that's really necessary in the first place. One could write a test script that would spawn dropbear (installed from a package or even compiled on the spot, as you have witnessed is very easily possible) and test against that.

(It is even possible to elevate to root on Travis, so one could just as well set up an sshd to listen on 127.0.0.1 and be done with it.)

Now, I know mc² is off-topic, but I want to mention that it's fun ​to write tests in it.

Yes, I've seen that ;-) and you know that I'm very enthusiastic about it in general, but I'm not sure whether this is the right approach in this particular case.

I've got the feeling that it's great for the integration kind of tests, but here we'd rather need to break up the scripts further, such that they can be unit tested against fake outputs from ls and friends from different systems (~normal and ~busyboxy Linux, ~BSD, ~Solaris).

Maybe if you have a deeper look at the scripts, you'll see what I mean.

[and]

Replying to zaytsev:

@and, since you broke it in #3599, do you think that in addition to providing a patch, you could cover the FISH scripts with unit tests as a punishment?

This shouldn't be too complicated (specifically, see chapters about Simple Tests), but this will make for the first time possible to change the venerable FISH scripts without panic fear:

​https://www.gnu.org/software/automake/manual/html_node/Tests.html

Right now, if you change anything there you have to do rigorous manual testing with different hosts and pray that nothing has got broken by the new changes...

Jep and thanks for the pointer, sounds great by "simple" shell check script(s), I put it on my TODO list (low urgency).

[woodsb02]

Please note that FreeBSD bug report 208104 has been raised to include the proposed patch to fix this issue in the FreeBSD 4.8.16 packages.
​https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=208104

[mooffie]

Replying to zaytsev-work:

Now, I know mc² is off-topic, but I want to mention that it's fun ​to write tests in it.

Yes, I've seen that ;-) and you know that I'm very enthusiastic about it in general, but I'm not sure whether this is the right approach in this particular case.

I've got the feeling that it's great for the integration kind of tests, but here we'd rather need to break up the scripts further, such that they can be unit tested against fake outputs from ls and friends from different systems (~normal and ~busyboxy Linux, ~BSD, ~Solaris).

Maybe if you have a deeper look at the scripts, you'll see what I mean.

Yes, I now see what you mean.

I also see, for example, that there's the same concern with ticket:3622: a test that tests it through the VFS might work (when the machine it runs on happens to have uzip compiled with MDY order) but some users will encounter a problem still (when using YMD order).

I'll use this opportunity to thank you for the way you compose your posts. They're laden with helpful overviews (e.g., ticket:2633, ticket:3622, ticket:2742, mentioning "integration kind of tests" vs others), with tips (e.g., mentioning dropbear etc, 'autoreconf' vs './autogen.sh') and that's just from today. Your posts certainly enlighten me (and others no doubt).

[zaytsev]

Ticket #3625 has been marked as a duplicate of this ticket.

[andrew_b]

Ticket #3631 has been marked as a duplicate of this ticket.

[andrew_b]

Ticket #3634 has been marked as a duplicate of this ticket.

[andrew_b]

Let fix this without tests.

[zaytsev-work]

I agree, will try today or tomorrow and create a separate ticket for tests, which can be acted upon later. I remember there was a related problem for patchfs, but that one can be tested easier.

[zaytsev]

I have created #3635 for FISH tests.

[zaytsev]

Branch: 3611_fish_ls_perl_fix
Initial changeset: 263012123cb1e4bf8201da00a9c248497b326076

[zaytsev]

Merged to master: 5ba97897cf119c8566dc328e8054a572350bda48