Enable tests for zsh, yash, mksh and ksh93u+m

[aartoni]

No description provided.

[aartoni]

@9ao9ai9ar this is my first time testing on a real GitHub instance, unfortunately the behavior seems to differ from the tool I've been using for local tests (i.e., act). Please give me some time to adjust a few things before merging.

[9ao9ai9ar]

No worries, I don't even know how to run and write for these tools, so I leave that all to you. Take your time.

[9ao9ai9ar]

Hi @aartoni, the scripts have been substantially updated since you opened this pull request. Are you interested in adding more runners to the workflow? The ksh, mksh and posh packages are also available from the official Ubuntu repositories.

Re how the scripts are tested, I am not sure if you should go so far as to test the exact exit status (just a =0 or >0 test is enough in my opinion), but if you do, it's probably more readable and less prone to breakage if you use the defined names for the exit status, which you could obtain like thus:

(. ./updater.sh 2>/dev/null && exit_status_definitions >./status_codes.txt) && 
    eval "$(cat ./status_codes.txt)"

And to use them:

if [ "$?" -eq "${_EX_OK:?}" ]; then
    # test logic
fi

There are also some behaviors you could tweak by setting certain environment variables (see the example advanced usage near the top of the files):

1. UTILITY__IMPLEMENTATION if using an alternative to the detected utility UTILITY is desired.
2. ARKENFOX_UPDATER_NAME/ARKENFOX_PREFS_CLEANER_NAME if the scripts were ever renamed.

Basically all the variables in all caps that do not start with an underscore are open to the users for tweaking.

[aartoni]

@9ao9ai9ar thank you for pinging me! I've added tests for mksh and ksh93u+m, this week I'd like to keep making small contributions until we reach a good state. For now, I've adopted your suggestion regarding env vars, although I didn't use eval since GitHub workflows require scripts to set them via the GITHUB_ENV var.

[aartoni]

Hey @9ao9ai9ar! We're almost ready for merging here, here's the list of things I want to finish before merging:

• enabling macos-latest and windows-latest tests (without all those shells);
• figuring out the best way of making the last test pass.

And here's a list of things I'd like to defer to another PR (after we merge yours into Arkenfox's):

• support for pbosh, ash, osh, hush, posh and ksh88;
• testing more features, such as: -n and -r;

[aartoni]

In the meantime, can I ask you to synchronize your branches with the latest edits from the upstream? Thanks in advance!

[9ao9ai9ar]

I'm wondering how would you test it on Windows? I'm not going down that rabbit hole by way of WSL, Cygwin or MSYS2, etc., but you're free to try.

For the last test, you may:

1. Compare checksums.
2. Test if user.js is non-empty ([ -s ./user.js ]).
3. Test if diff returns any output (probably not ideal).
4. Test if diff returns exit status 1 (probably not ideal either).

Please refer to the updated, topmost post at arkenfox#1908 for the support status of POSIX compliant shells: hush is not targeted, and I'm debating whether ksh88-based shells like pdksh and XPG4 sh should be targeted at all since they are crippled; likewise with bosh, which I'm still studying.

Tests for BusyBox ash and posh could be added as they're in the official APT repositories and have more users. You don't have to go overboard with automated test coverage since shells are just part of the equation: it's always better to test in the target OS directly, but since it's Microsoft's GitHub, they will make this hard for you.

[9ao9ai9ar]

Forgot to mention that you could install the ksh package instead of ksh93u+m and do away with the Set ksh93u+m shell step. ksh will be ksh93 for a long time, and we're only testing the latest versions anyway.

Do you mind if I do some editing before merging this? Also, I would like to turn this into manual run to be more environment friendly: most commits or pull requests will not be touching these shell scripts.

[aartoni]

I'm wondering how would you test it on Windows? I'm not going down that rabbit hole by way of WSL, Cygwin or MSYS2, etc., but you're free to try.

Windows might not be that hard thanks to the fact that the windows-latest image already includes bash.

For the last test, you may:

These are all good alternative, we're using diff atm, since it was the easiest. However I'm still wondering about the network operation, that is the download of the latest version of the user.js. I would really like to keep that out of the main test since a short network malfunction may incorrectly flag a PR as incorrect. I think I'll push your points 2. and 4. for now. Thanks for all the suggestions!

Please refer to the updated, topmost post at arkenfox#1908 for the support status of POSIX compliant shells

I'd say it's better if we remove the TODO notice before merging into the upstream. Then we can open a separate issue or PR with the support status you worked on in arkenfox#1908. I've removed hush for now.

Tests for BusyBox ash and posh could be added as they're in the official APT repositories and have more users.

I'll be adding ash in a few mins, thanks!

you could install the ksh package instead of ksh93u+m and do away with the Set ksh93u+m shell step

Thanks for pointing that out! I've pushed the suggested change!

Do you mind if I do some editing before merging this? Also, I would like to turn this into manual run to be more environment friendly: most commits or pull requests will not be touching these shell scripts.

I don't mind at all!

[aartoni]

@9ao9ai9ar ash added!

[9ao9ai9ar]

I'd say it's better if we remove the TODO notice before merging into the upstream. Then we can open a separate issue or PR with the support status you worked on in arkenfox#1908.

There are currently one TODO each in updater.sh and prefsCleaner.sh, and an unrealized subgoal "to-do" mentioned in my PR. After some thinking, here is what I would change:

• updater.sh TODO: turn this into an ordinary comment, as adding support for Python-style comments is not a trivial task with the current approach, and not implementing this obscure feature just means the diffs may not be minimal, which is harmless.
• prefsCleaner.sh TODO: give the notice in my PR post instead. I just want more eyes inspecting a crucial, reimplemented function which I might not have given enough thought.
• PR to-do: leave as-is. The fixes for the shortcomings are not workable without a complete test suite for POSIX shell commands and shell extensions, and are very unlikely to affect the average users anyway.

I will merge your PR and make the necessary changes sometime this week.

[9ao9ai9ar]

Hey @aartoni I just reworked the GitHub workflow and understood why you wanted me to synchronize my branches with the latest edits from the upstream - so that your last test with the diff could pass. I've removed that test since it requires constant synchronization with the upstream and therefore is brittle.

I've also run the tests using a Windows runner, but unfortunately the runner only supports Git Bash and WSL 1, and Git Bash doesn't include a mktemp, and possibly more utilities but I haven't checked, so it can't even run the main scripts.

Among the shells in your TODO, I've already added posh; ksh88 is superseded by mksh/oksh and it makes little sense to test this obsolescent shell in isolation without the whole OSes that still ship it and the accompanying userland utilities; I don't think official precompiled binaries or docker images for osh and bosh exist, and compiling them from source is an ongoing maintenance overhead due to changing dependencies, so I'm fine with leaving them out of the test.

[aartoni]

Hey @9ao9ai9ar thank you for taking over while I was away! I will have a look at the current state in the upcoming weeks