bonsai/harakit
4
4
Fork 3
Code Issues 41 Pull Requests 3 Projects 1 Releases 35 Activity

testing #96

Closed
emma wants to merge 0 commits from testing into main
pull from: testing
merge into: bonsai:main
Conversation 10 Commits - Files Changed -
emma commented 2024-04-24 15:00:28 -06:00
Owner
Copy Link
No description provided.
emma added the
enhancement
 label 2024-04-24 15:00:28 -06:00
emma added 5 commits 2024-04-24 15:00:28 -06:00
emma added 1 commit 2024-04-24 15:23:03 -06:00
emma added 1 commit 2024-04-24 15:25:30 -06:00
emma requested review from trinity 2024-04-24 15:27:43 -06:00
emma added 3 commits 2024-04-24 19:29:28 -06:00
emma added 1 commit 2024-04-25 17:21:58 -06:00
emma added 1 commit 2024-04-25 18:58:01 -06:00
trinity requested changes 2024-04-26 20:12:41 -06:00
trinity left a comment
Owner
Copy Link

I have some questions and concerns.

I have some questions and concerns.
tests/bonsai/false.sh Outdated
@ -0,0 +10,4 @@
. tests/bonsai/test_env
! false
! false -h
trinity commented 2024-04-26 19:57:11 -06:00
Owner
Copy Link

GNU false(1), a notoriously POSIX non-compliant implementation, would pass here, as (if I recall) it returns EXIT_FAILURE for improper usage. It shouldn't pass, though, because it prints a usage text to the standard output when usage help or version information are queried.

I think another good test would be false --help | wc -c | xargs test 0 =, though you should double check the exit statuses here before adding it.

GNU false(1), a notoriously POSIX non-compliant implementation, would pass here, as (if I recall) it returns `EXIT_FAILURE` for improper usage. It shouldn't pass, though, because [it prints a usage text to the standard output when usage help or version information are queried](https://git.savannah.gnu.org/cgit/coreutils.git/tree/src/true.c#n36). I think another good test would be `false --help | wc -c | xargs test 0 =`, though you should double check the exit statuses here before adding it.
emma commented 2024-04-26 20:53:33 -06:00
Author
Owner
Copy Link

Why would we test for GNU extensions in our implementation of true(1)?

Why would we test for GNU extensions in our implementation of `true(1)`?
trinity commented 2024-04-27 07:25:37 -06:00
Owner
Copy Link

It's testing for bloat. It drives home the fact that we will not compromise on making good, simple tools. If we did our tests would fail us.

It's testing for bloat. It drives home the fact that we *will not* compromise on making good, simple tools. If we did our tests would fail us.
emma commented 2024-04-27 13:04:00 -06:00
Author
Owner
Copy Link

But if we changed our minds, we could just change our test, and if someone made a PR with changes like that, they would just change the test too.

But if we changed our minds, we could just change our test, and if someone made a PR with changes like that, they would just change the test too.
trinity commented 2024-04-28 20:04:10 -06:00
Owner
Copy Link

That's true. I suppose such a test isn't necessary.

That's true. I suppose such a test isn't necessary.
emma marked this conversation as resolved
tests/bonsai/true.sh Outdated
@ -0,0 +1,13 @@
#!/bin/sh
trinity commented 2024-04-26 20:00:28 -06:00
Owner
Copy Link

Why is this separate from tests/posix-compat/true.sh?

Why is this separate from `tests/posix-compat/true.sh`?
emma commented 2024-04-26 20:52:44 -06:00
Author
Owner
Copy Link

This is true(1) from Bonsai and the one in posix-compat/true.sh is true(1p)

This is `true(1)` from Bonsai and the one in `posix-compat/true.sh` is `true(1p)`
trinity commented 2024-04-27 07:23:37 -06:00
Owner
Copy Link

Our true(1) and false(1) implementations are POSIX without extensions and shouldn't be extended. Maybe it would be better to only have them in the POSIX compatibility section.

Our true(1) and false(1) implementations are POSIX without extensions and shouldn't be extended. Maybe it would be better to only have them in the POSIX compatibility section.
emma commented 2024-04-27 13:06:20 -06:00
Author
Owner
Copy Link

It makes sense to have them in both because the true(1p) test script leverages our script; however, I have just realized that I have implemented the POSIX test suite in a different way than I originally imagined and will make more changes before we can continue this conversation.

It makes sense to have them in both because the true(1p) test script *leverages* our script; however, I have just realized that I have implemented the POSIX test suite in a different way than I originally imagined and will make more changes before we can continue this conversation.
emma marked this conversation as resolved
tests/test.sh Outdated
@ -0,0 +17,4 @@
exit 1
fi
printf "Starting Bonsai testing.\n\n"
trinity commented 2024-04-26 20:10:57 -06:00
Owner
Copy Link

This seems ineligant. Programmatically print the test suite name at tests/$suite/Name, falling back to the folder name itself.

for suite in tests/*; do
    if ! scrut -d "$suite"
        then continue
    fi
    s="$suite" sh -c '
            scrut -e "$s"/Name \
                && mm <"$s/Name" \
                || printf "%s\n" "$s"'
        | xargs printf 'Testing <%s>...\n'
    # testing stuff
done
This seems ineligant. Programmatically print the test suite name at `tests/$suite/Name`, falling back to the folder name itself. ```sh for suite in tests/*; do if ! scrut -d "$suite" then continue fi s="$suite" sh -c ' scrut -e "$s"/Name \ && mm <"$s/Name" \ || printf "%s\n" "$s"' | xargs printf 'Testing <%s>...\n' # testing stuff done
emma marked this conversation as resolved
tests/test.sh Outdated
@ -0,0 +28,4 @@
printf '\n'
done
if ! ls Makefile >/dev/null 2>&1
trinity commented 2024-04-26 20:12:18 -06:00
Owner
Copy Link

This should go before the previous for loop as it will crash faster on incorrect invocation.

This should go before the previous `for` loop as it will crash faster on incorrect invocation.
emma marked this conversation as resolved
trinity commented 2024-04-26 20:14:15 -06:00
Owner
Copy Link

Goodness, I just saw the WIP: . Sorry for prematurely reviewing. It makes sense then that it seemed that this was unfinished. I really like where this is headed though.

Goodness, I just saw the `WIP: `. Sorry for prematurely reviewing. It makes sense then that it seemed that this was unfinished. I really like where this is headed though.
emma added 1 commit 2024-04-26 20:56:29 -06:00
emma commented 2024-04-26 20:56:52 -06:00
Author
Owner
Copy Link

Goodness, I just saw the WIP: . Sorry for prematurely reviewing. It makes sense then that it seemed that this was unfinished. I really like where this is headed though.

I requested it.

> Goodness, I just saw the `WIP: `. Sorry for prematurely reviewing. It makes sense then that it seemed that this was unfinished. I really like where this is headed though. I requested it.
emma added 1 commit 2024-04-27 15:17:53 -06:00
emma added 1 commit 2024-04-27 15:19:04 -06:00
emma added 1 commit 2024-05-27 22:08:18 -06:00
emma added 1 commit 2024-05-27 22:13:43 -06:00
emma added 1 commit 2024-05-27 22:18:20 -06:00
emma added 1 commit 2024-05-27 22:24:49 -06:00
emma added 1 commit 2024-05-27 22:38:09 -06:00
emma requested review from silt 2024-05-29 19:34:48 -06:00
emma requested review from trinity 2024-06-02 18:51:22 -06:00
emma added 2 commits 2024-06-07 23:31:25 -06:00
emma added 1 commit 2024-06-29 08:06:08 -06:00
emma commented 2024-06-29 20:00:50 -06:00
Author
Owner
Copy Link

No pull requests containing new utilities will be merged until this PR is merged.

#93 #102 #103 #127

No pull requests containing new utilities will be merged until this PR is merged. #93 #102 #103 #127
emma added a new dependency 2024-06-29 22:02:10 -06:00
#103 eep(1)
emma added a new dependency 2024-06-29 22:02:18 -06:00
#93 peek(1)
emma added a new dependency 2024-06-29 22:02:28 -06:00
#102 Replace str(1) with stris(1)
emma added a new dependency 2024-06-29 22:02:36 -06:00
#127 WIP: scroll(1)
emma added 1 commit 2024-07-07 19:20:46 -06:00
trinity added 3 commits 2024-07-08 22:25:30 -06:00
emma added a new dependency 2024-07-20 10:06:27 -06:00
#146 Replace scrut(1) with fileis(1)
emma added 1 commit 2024-08-02 17:29:44 -06:00
trinity added 1 commit 2024-08-02 18:23:03 -06:00
trinity added 1 commit 2024-08-02 18:28:34 -06:00
emma added 1 commit 2024-08-02 19:37:45 -06:00
trinity added 3 commits 2024-08-04 08:47:46 -06:00
emma added 1 commit 2024-08-05 13:47:23 -06:00
trinity added 3 commits 2024-08-05 15:01:38 -06:00
trinity added 2 commits 2024-08-07 08:34:45 -06:00
trinity added 1 commit 2024-08-07 08:35:32 -06:00
trinity commented 2024-08-07 09:55:26 -06:00
Owner
Copy Link

All utilities now have tests. Some are better than others, but the foundation is there.

All utilities now have tests. Some are better than others, but the foundation is there.
emma changed title from WIP: testing to testing 2024-08-07 12:32:05 -06:00
trinity requested changes 2024-08-07 20:22:22 -06:00
trinity left a comment
Owner
Copy Link

Just some notes regarding inconsistencies.

Just some notes regarding inconsistencies.
tests/README Outdated
@ -0,0 +4,4 @@
.
├── bonsai
│   ├── test_env
│   ├── dj.sh
trinity commented 2024-08-07 20:14:33 -06:00
Owner
Copy Link

This needs to be updated.

This needs to be updated.
emma marked this conversation as resolved
tests/bonsai/hru.mk Outdated
@ -0,0 +7,4 @@
NAME = hru
TARGET = $(NAME)_tests
BINARY = $(BIN)/$(NAME)
trinity commented 2024-08-07 20:16:32 -06:00
Owner
Copy Link

Are these variables necessary? Should we use this pattern everywhere?

Are these variables necessary? Should we use this pattern everywhere?
emma commented 2024-08-07 20:42:13 -06:00
Author
Owner
Copy Link

No, they’re a holdover from a previous draft.

No, they’re a holdover from a previous draft.
emma marked this conversation as resolved
tests/bonsai/hru.mk
@ -0,0 +28,4 @@
.PHONY: hru_regressions
hru_regressions: $(BIN)/hru
n=1; \
while true; \
trinity commented 2024-08-07 20:18:44 -06:00
Owner
Copy Link

Could this be while printf '%s\n' "$$n" | $(BIN)/hru; do n=$$(($$n * 10))"; done;?

Could this be `while printf '%s\n' "$$n" | $(BIN)/hru; do n=$$(($$n * 10))"; done;`?
emma commented 2024-08-07 20:42:32 -06:00
Author
Owner
Copy Link

Seems not to work for me.

Seems not to work for me.
emma marked this conversation as resolved
emma added 1 commit 2024-08-07 20:41:52 -06:00
emma added 1 commit 2024-08-07 20:45:30 -06:00
trinity commented 2024-08-07 21:14:09 -06:00
Owner
Copy Link

The current Makefile just builds and tests true(1) for me. The following change seems to fix this:

diff --git a/Makefile b/Makefile
index 6c6d555..906e7f4 100644
--- a/Makefile
+++ b/Makefile
@@ -31,13 +31,13 @@ RUSTLIBS = --extern getopt=build/o/libgetopt.rlib \
        --extern strerror=build/o/libstrerror.rlib
 CFLAGS += -I$(SYSEXITS)
 
+.PHONY: default
+default: all test
+
 # testing requires the absolute path to the bin directory set
 BIN = build/bin
 include tests/tests.mk
 
-.PHONY: default
-default: all test
-
 .PHONY: all
 all: dj false fop hru intcmp mm npc rpn scrut str strcmp swab true

but I haven't committed it because this may be a poor solution (Emma, what do you think?).

I edited the fop_functionality test to print the actual result versus the expected:

diff --git a/tests/bonsai/fop.mk b/tests/bonsai/fop.mk
index 9013308..c7d1033 100755
--- a/tests/bonsai/fop.mk
+++ b/tests/bonsai/fop.mk
@@ -27,5 +27,7 @@ fop_fail: $(BIN)/fop
 
 .PHONY: fop_functionality
 fop_functionality: $(BIN)/fop
-       test "$$(printf 'test1\036test1\036test1\n' | $(BIN)/fop 1 sed 's/1/4/g')" \
-               = "$$(printf 'test1\036test4\036test1\n')"
+       printf 'test1\036test1\036test1\n' \
+               | $(BIN)/fop 1 sed s/1/4/g \
+               | tee /dev/stderr \
+               | xargs -I out test "$$(printf 'test1\036test4\036test1\n')"

but I don't know if you approve of this style. Note the tee(1p) though.

I've edited this comment to remove a report of a fop(1) test failure that's fixed when using fop(1) from the main branch.

The current `Makefile` just builds and tests true(1) for me. The following change seems to fix this: ``` diff --git a/Makefile b/Makefile index 6c6d555..906e7f4 100644 --- a/Makefile +++ b/Makefile @@ -31,13 +31,13 @@ RUSTLIBS = --extern getopt=build/o/libgetopt.rlib \ --extern strerror=build/o/libstrerror.rlib CFLAGS += -I$(SYSEXITS) +.PHONY: default +default: all test + # testing requires the absolute path to the bin directory set BIN = build/bin include tests/tests.mk -.PHONY: default -default: all test - .PHONY: all all: dj false fop hru intcmp mm npc rpn scrut str strcmp swab true ``` but I haven't committed it because this may be a poor solution (Emma, what do you think?). I edited the `fop_functionality` test to print the actual result versus the expected: ``` diff --git a/tests/bonsai/fop.mk b/tests/bonsai/fop.mk index 9013308..c7d1033 100755 --- a/tests/bonsai/fop.mk +++ b/tests/bonsai/fop.mk @@ -27,5 +27,7 @@ fop_fail: $(BIN)/fop .PHONY: fop_functionality fop_functionality: $(BIN)/fop - test "$$(printf 'test1\036test1\036test1\n' | $(BIN)/fop 1 sed 's/1/4/g')" \ - = "$$(printf 'test1\036test4\036test1\n')" + printf 'test1\036test1\036test1\n' \ + | $(BIN)/fop 1 sed s/1/4/g \ + | tee /dev/stderr \ + | xargs -I out test "$$(printf 'test1\036test4\036test1\n')" ``` but I don't know if you approve of this style. Note the tee(1p) though. I've edited this comment to remove a report of a fop(1) test failure that's fixed when using fop(1) from the `main` branch.
emma commented 2024-08-07 21:56:43 -06:00
Author
Owner
Copy Link

The current Makefile just builds and tests true(1) for me. The following change seems to fix this:

diff --git a/Makefile b/Makefile
index 6c6d555..906e7f4 100644
--- a/Makefile
+++ b/Makefile
@@ -31,13 +31,13 @@ RUSTLIBS = --extern getopt=build/o/libgetopt.rlib \
        --extern strerror=build/o/libstrerror.rlib
 CFLAGS += -I$(SYSEXITS)
 
+.PHONY: default
+default: all test
+
 # testing requires the absolute path to the bin directory set
 BIN = build/bin
 include tests/tests.mk
 
-.PHONY: default
-default: all test
-
 .PHONY: all
 all: dj false fop hru intcmp mm npc rpn scrut str strcmp swab true

but I haven't committed it because this may be a poor solution (Emma, what do you think?).

I cannot reproduce this behavior with either pdpmake(1) or OpenBSD make(1).

> The current `Makefile` just builds and tests true(1) for me. The following change seems to fix this: > > ``` > diff --git a/Makefile b/Makefile > index 6c6d555..906e7f4 100644 > --- a/Makefile > +++ b/Makefile > @@ -31,13 +31,13 @@ RUSTLIBS = --extern getopt=build/o/libgetopt.rlib \ > --extern strerror=build/o/libstrerror.rlib > CFLAGS += -I$(SYSEXITS) > > +.PHONY: default > +default: all test > + > # testing requires the absolute path to the bin directory set > BIN = build/bin > include tests/tests.mk > > -.PHONY: default > -default: all test > - > .PHONY: all > all: dj false fop hru intcmp mm npc rpn scrut str strcmp swab true > ``` > > but I haven't committed it because this may be a poor solution (Emma, what do you think?). I cannot reproduce this behavior with either `pdpmake(1)` or OpenBSD `make(1)`.
emma added 1 commit 2024-08-07 22:03:40 -06:00
trinity commented 2024-08-09 09:06:28 -06:00
Owner
Copy Link

I cannot reproduce this behavior with either pdpmake(1) or OpenBSD make(1).

OpenBSD/7.5 arm64 make(1):

$ uname -a
OpenBSD laika.my.domain 7.5 GENERIC#97 arm64
$ command -v make
/usr/bin/make
$ make clean
rm -rf build dist
$ make
mkdir -p build/bin build/docs build/include build/lib build/o build/test
cc -O2 -pipe  -I/usr/include/ -o build/bin/true src/true.c
build/bin/true

pdpmake(1) compiled on OpenBSD:

$ alias make=pdpmake
$ make clean
rm -rf build dist
$ git restore .
$ make
pdpmake: nothing to be done for /dev/full

The default target for a Makefile (make run with no arguments) is the first target that appears in the Makefile. The first target that appears in the Makefile here is, apparently, default, a .PHONY that depends on all and test. That default target is the intended behavior of the make invocation.

However before default there's an include tests/tests.mk. That file includes all the test files found in tests/. It makes sense that those targets would be considered, as a part of dependency resolution, before default. This deviation from intended behavior makes logical sense and is being demonstrated by these make invocations shown above. My fix based on this understanding works conceptually and in practice.

I'm not sure why your system is behaving differently. Could you git status to see if the repository is any different?

> I cannot reproduce this behavior with either `pdpmake(1)` or OpenBSD `make(1)`. OpenBSD/7.5 arm64 make(1): ``` $ uname -a OpenBSD laika.my.domain 7.5 GENERIC#97 arm64 $ command -v make /usr/bin/make $ make clean rm -rf build dist $ make mkdir -p build/bin build/docs build/include build/lib build/o build/test cc -O2 -pipe -I/usr/include/ -o build/bin/true src/true.c build/bin/true ``` pdpmake(1) compiled on OpenBSD: ``` $ alias make=pdpmake $ make clean rm -rf build dist $ git restore . $ make pdpmake: nothing to be done for /dev/full ``` The default target for a Makefile (`make` run with no arguments) is the first target that appears in the Makefile. The first target that appears in the Makefile here is, apparently, `default`, a `.PHONY` that depends on `all` and `test`. That `default` target is the intended behavior of the `make` invocation. However before `default` there's an `include tests/tests.mk`. That file includes all the test files found in `tests/`. It makes sense that those targets would be considered, as a part of dependency resolution, before `default`. This deviation from intended behavior makes logical sense and is being demonstrated by these `make` invocations shown above. My fix based on this understanding works conceptually and in practice. I'm not sure why your system is behaving differently. Could you `git status` to see if the repository is any different?
emma added 1 commit 2024-08-09 18:00:51 -06:00
emma commented 2024-08-09 23:29:26 -06:00
Author
Owner
Copy Link

Merged.

Merged.
emma closed this pull request 2024-08-09 23:29:26 -06:00
emma deleted branch testing 2024-08-09 23:29:31 -06:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
silt
trinity
Labels
Clear labels
bug
Something is not working
duplicate
This issue or pull request already exists
enhancement
New feature
help wanted
Need some help
invalid
Something is wrong
joke
you are silly!
question
More information is needed
wontfix
This won't be fixed
No Label
enhancement
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
emma nitori silt trinity
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Blocks
#93 peek(1)
bonsai/harakit
#102 Replace str(1) with stris(1)
bonsai/harakit
#103 eep(1)
bonsai/harakit
#127 WIP: scroll(1)
bonsai/harakit
#146 Replace scrut(1) with fileis(1)
bonsai/harakit
Reference: bonsai/harakit#96
No description provided.
Delete Branch "testing"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?