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

Implement tests #10

Open
adnan360 opened this issue Mar 29, 2021 · 26 comments
Open

Implement tests #10

adnan360 opened this issue Mar 29, 2021 · 26 comments

Comments

@adnan360
Copy link

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.

@adnan360 adnan360 mentioned this issue Mar 29, 2021
@Phate6660
Copy link
Owner

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:

  • output formatting
  • tests
  • decimals

@Phate6660
Copy link
Owner

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

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
Copy link
Owner

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

@Phate6660
Copy link
Owner

Fixed.

@adnan360
Copy link
Author

... 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
Copy link
Owner

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
Copy link
Owner

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:

  • Basic tests for parenthesis, exponents, multiplication, division, addition subtraction, and PEMDAS
  • Will output the result and what's expected if it's wrong
  • Will create an appropriate file in debug/ for the test case
  • Will write debug output (+ info from bash -x) into the appropriate file
    Example output of how it currently looks is:
$ ./test
GOOD -- The basic parenthesis test has passed.
GOOD -- The basic exponent test has passed.
GOOD -- The basic multiplication test has passed.
GOOD -- The basic division test has passed.
GOOD -- The basic addition test has passed.
GOOD -- The basic subtraction test has passed.
BAD  -- The result of the basic PEMDAS test is "2,512", when it should be "512".
    (A debug file is available at 'debug/basi_pemdas_test-debug-output'.)
BAD  -- The result of the second basic PEMDAS test is "5,024", when it should be "1024".
    (A debug file is available at 'debug/bas_pemdas2_test-debug-output'.)

@adnan360
Copy link
Author

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. 💯

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
Copy link
Author

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

@Phate6660
Copy link
Owner

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Author

adnan360 commented Mar 31, 2021

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
Copy link
Owner

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

@Phate6660
Copy link
Owner

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
Copy link
Owner

$ ./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
Copy link
Author

adnan360 commented Apr 2, 2021

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. 👍

@adnan360
Copy link
Author

adnan360 commented Apr 2, 2021

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
Copy link
Owner

Phate6660 commented Apr 2, 2021

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

🤦 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. 👍

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
Copy link
Owner

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

Fixed. :)

@Phate6660
Copy link
Owner

Phate6660 commented Apr 3, 2021

$ ./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
Copy link
Owner

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

@adnan360
Copy link
Author

adnan360 commented Apr 4, 2021

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

No problem, happens to everyone. :)

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 a pull request may close this issue.

2 participants