Phate6660 / bcalc

A pure-bash calculator in which I even wrote my own lexer and parser for.
Other
5 stars 1 forks source link

Implement tests #10

Open adnan360 opened 3 years ago

adnan360 commented 3 years ago

We usually have to test ourselves when we have new features to see if that broke any old features. What if we had tests to do that automatically for us?

Found out that bats is a good testing system for bash. I have some tests written for it in a branch here.

I'll push a PR shortly for your consideration, but I was thinking if bats is even too much for this project. I mean, if it can be done with pure bash only that would be awesome in a way.

Phate6660 commented 3 years ago

I would rather not use bats for this, or any framework for that matter.

I will implement testing myself in pure bash.

That will be next on the list.

So my order now is:

Phate6660 commented 3 years ago

I have begun implementing a testing script here, as I've hit a bit of a snag with the output formatting: https://github.com/Phate6660/bcalc/tree/tests

adnan360 commented 3 years ago

I like it a lot. Especially the fact that it's just one file and it runs the tests when the file is run. Simple.

One thing I think would be better if we changed the debug outputs. It outputs directly to repo root and git sees the debug files as changes. We can have a separate folder named debug or something and add it in .gitignore.

To keep the empty dir in repo but ignore the files inside, I usually create a file named .keep inside it and add something like this to .gitignore:

debug/*
!*/.keep

Or to keep it simple, mkdir can be added at the beginning of the script to make sure the debug dir is created and the debug dir can be in a variable which can be used inside tests.

adnan360 commented 3 years ago

Also, I think adding something like this would allow it to be used from outside the dir:

repo_root="$(dirname $(realpath $0))"
exe="${repo_root}/bcalc"
Phate6660 commented 3 years ago

It outputs directly to repo root and git sees the debug files as changes.

It doesn't? I added this to the .gitignore:

# Debug files created by the test script
*debug*

And it doesn't pick them up.

Also, I think adding something like this would allow it to be used from outside the dir:

I don't really see the point in running it out of the repo though. If you're testing it, it's most likely because you're actively developing it and in that case you're gonna be in the directory anyways.

Phate6660 commented 3 years ago

It actually does, I'm an idiot and forgot to commit the .gitignore change.

Phate6660 commented 3 years ago

Fixed.

adnan360 commented 3 years ago

... forgot to commit the .gitignore change.

I see. Great. It's now working, debug files are not seen by git.

But still I think if there are many failures for some reason it will look like a mess. A separate directory would be nice to have.

Phate6660 commented 3 years ago

You are right about that for sure. I'll edit the test script to make the dir if it doesn't exist, output to that dir, and make sure the the dir is ignored by git.

Phate6660 commented 3 years ago

I think it's good enough now to be merged into master, what do you think? I feel like it's a solid base (and I can always add more tests when necessary). To recap, ./test does:

adnan360 commented 3 years ago

Yeah. I think it's better than I expected. Single file, no deps to test. Just needs bash and works with it. Best fit for a project like this. :100:

One small thing I noticed:

if ! [ -d "./debug" ]; then mkdir debug; else :; fi

Maybe this else :; part is not necessary. If that's ok for some reason, I think you can merge and close this issue.

adnan360 commented 3 years ago

Oh and another thing. I think you should add a parenthesis inside parenthesis test.

Phate6660 commented 3 years ago

Maybe this else :; part is not necessary.

Oh shoot, yeah you're right. I always feel compelled to write an else case. And in this case it is definitely not needed.

Phate6660 commented 3 years ago

Oh and another thing. I think you should add a parenthesis inside parenthesis test.

The parser hasn't been re-written yet to accomodate for that. Once it has, and I confirm it as a working feature, I will add a test for it.

Phate6660 commented 3 years ago

I'll keep this issue open until I feel we we have enough tests. We can use this issue to come up with more tests for bcalc. I will merge the PR now, I feel like it's time.

adnan360 commented 3 years ago

I'll keep this issue open until I feel we we have enough tests. We can use this issue to come up with more tests for bcalc.

I think that's a great idea. I had some tests in my bats based PR if you want to check out.

Now that I checked it, I think modulus may have been missed in your tests. Another test I had is the decimal fail test. Maybe this can be used for testing errors as well, for checking if it triggers error messages on faulty user inputs. e.g.

$ ./bcalc '2++2'
./bcalc: line 175: 2+2+: syntax error: operand expected (error token is "+")
$ ./bcalc '2+2='
Only digits, parenthesis, '^', '*', '%', '/', '+', and '-', are supported.
'=' is currently unsupported.
$ ./bcalc '2_2'
Only digits, parenthesis, '^', '*', '%', '/', '+', and '-', are supported.
'_' is currently unsupported.
$ ./bcalc '^2'
./bcalc: line 175: 2**: syntax error: operand expected (error token is "**")
Phate6660 commented 3 years ago

Just made both basic PEMDAS tests pass. I have also removed formatted output until I get the output-formatting branch done.

Phate6660 commented 3 years ago

I just realized I forgot to respond to your last comment. \/\/-_-\/\/ Yep, that's on my list next. Tests for error messages, as well as adding more detailed errors depending on the circumstance.

Phate6660 commented 3 years ago

$ ./bcalc '2++2' ./bcalc: line 175: 2+2+: syntax error: operand expected (error token is "+")

Added an error message for this.

$ ./bcalc '^2' ./bcalc: line 175: 2: syntax error: operand expected (error token is "")

This as well.


What's wrong with the other ones though? The error messages for the others seem just as I intended. Do they need to be improved?

adnan360 commented 3 years ago

What's wrong with the other ones though? The error messages for the others seem just as I intended. Do they need to be improved?

No, no problem there. I just presented those outputs to be added to tests.

Now that the missing error messages are added, I think tests would be much more extensive and dependable. I really like where this is going. I can't believe all this is being done on pure bash. :+1:

adnan360 commented 3 years ago

Another situation I think should have an error message is this one:

$ ./bcalc '2+2+'
./bcalc: line 210: 2+2+: syntax error: operand expected (error token is "+")

And just thought about this one, what if someone has a calculation starting with a negative number? Like this...

$ ./bcalc '-2+2'
Currently other notations are unsupported, please start with a number.
Phate6660 commented 3 years ago

what if someone has a calculation starting with a negative number?

:facepalm: My bad, that totally slipped my mind. I can fix that real quick.

Another situation I think should have an error message is this one:

I can think of how I might go about it, but I'm not certain. So this one may or may not take a bit.

No, no problem there. I just presented those outputs to be added to tests.

Ah, oke doke.

Now that the missing error messages are added, I think tests would be much more extensive and dependable. I really like where this is going. I can't believe all this is being done on pure bash. :+1:

I agree. And same! I wasn't sure how far I'd get with pure bash either; but the more I use it and research it, the more powerful I find it to be.

Phate6660 commented 3 years ago

$ ./bcalc '-2+2' Currently other notations are unsupported, please start with a number.

Fixed. :)

Phate6660 commented 3 years ago

$ ./bcalc '2+2+' ./bcalc: line 210: 2+2+: syntax error: operand expected (error token is "+")

Added an error message! The answer was surprisingly simple. Which means that this was either like a stroke of luck, or I managed to forget something and broke other use cases.

Phate6660 commented 3 years ago

I was right, I broke starting with a negative number. However I have fixed it now. :)

adnan360 commented 3 years ago

I was right, I broke starting with a negative number. However I have fixed it now. :)

No problem, happens to everyone. :)