Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve test shell scripts #44

Merged
merged 12 commits into from Jul 25, 2023
Merged

Improve test shell scripts #44

merged 12 commits into from Jul 25, 2023

Conversation

pabs3
Copy link
Contributor

@pabs3 pabs3 commented Jul 20, 2023

  • Standardise on set -eux for all test shell scripts
  • Run git-mediate via $PATH instead of a location stored in a variable
  • Avoid passing options to echo
  • Use = instead of == within [ ] tests
  • Quote references to shell variables
  • Use braces around references to shell variables
  • Set the test user home directory to the test directories
  • Wrap multi-command shell statements more
  • Add shebangs to the shell scripts
  • Consistently single-quote static strings containing spaces
  • Ensure the tests run in a clean directory and clean it on exit
  • Print the current directory in all tests for consistency
  • Indicate successful completion of tests

Copy link
Collaborator

@yairchu yairchu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!
I've cherry-picked one of the commits (which is actually necessary for the tests to properly run on macOS) and for some of the other changes the tests are not working for me and I suggested a few changes.

test/config/test Outdated Show resolved Hide resolved
test/config/test Show resolved Hide resolved
test/spaces/test Outdated Show resolved Hide resolved
pabs3 added 12 commits July 25, 2023 20:17
The command printing (-x) is needed for all the tests.

The short form is more readable.
Makes it easier to override which git-mediate will be run,
especially in automatic tests of distro packages.
The == operator is undefined in POSIX shell.

Suggested-by: shellcheck
See-also: https://www.shellcheck.net/wiki/SC3014
Otherwise they will be subject to globbing and word splitting.

Suggested-by: shellcheck
See-also: https://www.shellcheck.net/wiki/SC2086
Avoids altering developer home directories when running
tests that alter the per-user git configuration files.
Avoids overly long lines.

Suggested-by: bashate
This allows them to be run directly instead of be prefixed with the shell.

They are now POSIX shell scripts so use /bin/sh as the shell interpreter.
This way they show up as strings in editor syntax highlighting.
The trap builtin sets a cleanup function to be run at exit.

Call the cleanup function before the tests start too.

Add more files to be cleaned up too.
@pabs3
Copy link
Contributor Author

pabs3 commented Jul 25, 2023

In the process of reviewing the test output, I noticed a bug around handling of whitespace chars in filenames (#45). Unfortunately that didn't cause failure of the spaces test. Can you think of anything I could do to fix that?

@yairchu yairchu merged commit 1a7cb9e into Peaker:master Jul 25, 2023
@yairchu
Copy link
Collaborator

yairchu commented Jul 25, 2023

Thanks!

In the process of reviewing the test output, I noticed a bug around handling of whitespace chars in filenames (#45). Unfortunately that didn't cause failure of the spaces test. Can you think of anything I could do to fix that?

It's good to know that there's a -z mode which is recommended for machine parsing. I'll look into switching into it because it will probably only be more robust. When doing that I'll also try to repro the problem in a test, have no ideas at the moment.

@pabs3
Copy link
Contributor Author

pabs3 commented Jul 25, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants