Document instructions on how to run the tests #7

Closed
yaccz opened this Issue Feb 5, 2013 · 29 comments

Comments

Projects
None yet
3 participants

yaccz commented Feb 5, 2013

No description provided.

yaccz commented Feb 5, 2013

Also

  1. it would be nice to use one of the common method's like python setup.py test or nosetests.
  2. and do not use python3 directly. I'm not sure how it works on other distros but in gentoo we run just python which will then execute the proper python-X.Y
Owner

ymattw commented Feb 5, 2013

  1. You should know people hates to write unit test for code with simple logic :)
  2. I am using python3 explicitly in Makefile because I want test with both python2 and python3. I might need to add python2 explicitly too

I do not document how to test as I thought only myself care that, can I ask why you expect a document for how to test, any special reason?

yaccz commented Feb 5, 2013

  1. Dont care. If the tests exists I'm gonna execute them.
  2. I'm just saying the python versions shouldn't be hardcoded. I might send you a pull request some time later on this one
  3. It just should be documented. If I install your software and see it have tests, I run them to check it installed allright on my system. In order to run your tests myself I know have to go through the code and figure out how is it supposted to work instead of just reading the correct way on how to execute the tests, written by upstream.

I suspect you just run make test and if the return code is 0 you assume, the tests passed. You also just test that the code doesnt crash, right?

Owner

ymattw commented Feb 5, 2013

I just make test before commit, I don't have test for post install check. You are right the test is manually verified with my eyes, not easy to do that programically as it is an interact tool. Pull request welcome.

@ymattw ymattw added a commit that referenced this issue Feb 6, 2013

@ymattw ymattw more document for issue #7 f6edf89

@yaccz yaccz added a commit to yaccz/cdiff that referenced this issue Feb 12, 2013

@yaccz yaccz + --color option refs #7 4f29fc8

@yaccz yaccz added a commit to yaccz/cdiff that referenced this issue Feb 12, 2013

@yaccz yaccz automated tests refs #7 dbe9ebf

@yaccz yaccz added a commit to yaccz/cdiff that referenced this issue Feb 12, 2013

@yaccz yaccz make the makefile use automated tests refs #7 78a3273

yaccz commented Feb 12, 2013

So, how does this look like?

Owner

ymattw commented Feb 12, 2013

That's right.

On Tuesday, February 12, 2013, yaccz wrote:

https://github.com/ymattw/cdiff/blob/master/Makefile#L21 This part of
tests is to verify this
https://github.com/ymattw/cdiff/blob/master/cdiff.py#L619 right?


Reply to this email directly or view it on GitHubhttps://github.com/ymattw/cdiff/issues/7#issuecomment-13428768.

Owner

ymattw commented Feb 12, 2013

Thanks, Actually I did the same to project colordiff. I was considering use 'script' to force dump ansi color chars, as I am not sure when --color can be used except for test. I will look into your changes when back to a physical keyboard.

yaccz commented Feb 13, 2013

I'm not sure either but could be helpfull in some special cases (use or environments). I made this option after the one in grep.

Owner

ymattw commented Feb 13, 2013

Option --color is fine, it gives the possibility to use a pager other than less. I have several consideration on the changes on tests.

  • Maintaining the expected output file would be a problem, if I changed output format, I have to dump new format and use it as expected output, the extra effort does not bring us more value because it still depends on human eyes at the beginning
  • Introducing shunit2 is too much for cdiff right now, I even prefer to implement our own assertion and main proc if an acceptance tests is really required for this simple tool
  • -w N tests are missing, execution bit of tests/acceptance/tests.sh is missing too

In summary, my friend, I don't want to make cdiff becomes any big or complex, I am fine to take 4f29fc8 (send pull request please), others are ok too if we can keep them simple.

yaccz commented Feb 13, 2013

well

  1. Every tests depened on human verification at first. The point is you can make further changes, run tests automaticaly and be confident you have not broken existing functionality.
  2. shunit2 is required just for testing and it's pretty small dependency.
  3. -w N tests weren't even before I made these.
Owner

ymattw commented Feb 13, 2013

okay i forgot -w N wasn't there, i agree with you on 1, but prefer to implement simpler script to remove shunit2 dep.

Owner

ymattw commented Feb 13, 2013

Send pull request for 4f29fc8 please, I will implement others in my own way.

yaccz commented Feb 13, 2013

Go ahead, invent another wheel

Owner

ymattw commented Feb 13, 2013

don't you think there are reasons people reinventing wheels? i hate that too but i have my own choice when balancing between simplification and functionality.

TomWij commented Feb 13, 2013

not easy to do that programically as it is an interact tool

Checking cdiff file anotherFile | md5sum against a known MD5sum, for which you have guaranteed that the output of cdiff looks good by eye; is a priceless way to do it.

yaccz commented Feb 14, 2013

@TomWij that's kind of clever but not better. That way:

  1. the test will tell you only that it is broken, not where it broke.
  2. And while doing code review, or someone just interested browsing the tests, or whatever, you can cat expected_out and see exactly what should come out of cdiff.

@yaccz yaccz added a commit to yaccz/cdiff that referenced this issue Feb 14, 2013

@yaccz yaccz fix first value is expected one refs #7 f5668b6

@yaccz yaccz added a commit to yaccz/cdiff that referenced this issue Feb 14, 2013

@yaccz yaccz show only if the files differ, not the diff itself refs #7 316a758
Owner

ymattw commented Feb 14, 2013

@yaccz I respect your professional attitude on this issue, could you update README.rst as well and send me a pull request? I am not sure which one is homepage of shunit2.

TomWij commented Feb 14, 2013

Jan Matějka, I wasn't addressing you and I thought we were on par. I was addressing the statement where @ymattw has shown that he doesn't know or understand automated testing. Anyhow, I'll address your points anyway, they might benefit ymattw too:

(1) the test will tell you only that it is broken, not where it broke.

Tests only test one thing, thus there will be one thing broken so you know exactly what needs to be fixed.

(2) cat expected_out and see exactly what should come out of cdiff.

It is a waste of time to do this all manually when it can be automated, as that makes the tests actually useful; once you have a checksum mismatch, that is the moment you need to output them so you can look into the difference.

You have stated before:

The tests require manual inspection of the output and are not included in the ebuild.

This is what we're trying to solve here, right? Automated testing would give the best outcome, it makes you spot things you don't see on your local system...

If you don't go for this, excuse me, I'm just following this so I know whether to let the ebuild call automated tests or not (and to suggest them to be used).

With kind regards,

Tom Wijsman
Gentoo Developer

yaccz commented Feb 14, 2013

@TomWij

This is what we're trying to solve here, right?

right, I was just comparing the current approach that I implemented vs. your suggestion to use checksums.

Tests only test one thing, thus there will be one thing broken so you know exactly what needs to be fixed.

That's true only for unit tests. I mean, in this case, if the test with checksum fails, you know what file failed. But to see what part of the output causes the fail, you need to

  1. checkout older version
  2. generate the output
  3. checkout the current (failing) version
  4. and compare the outputs

while when using the expected output in test directly, you go straight for 4.

@ymattw will do

TomWij commented Feb 14, 2013

Ah right, the checksum was suggested for speed purposes only anyway. You're right, I haven't looked at the commits..

Owner

ymattw commented Feb 15, 2013

shunit2 isn't that good, I insist to fix in my own way (one case tests one thing). That's ok if you think I am reinventing another wheel, or think I don't know or understand automated testing. I appreciate your feedback anyway.

yaccz commented Feb 15, 2013

shunit2 isn't that good,

why?

Owner

ymattw commented Feb 15, 2013

It is too big for me when I only need one assertion function and a kick-off driver, it might be good for large projects but not a good fit for cdiff, that's my opinion.

Anyway... if you look into my last commit, I just implemented your idea in my own way, I hope you are ok to this. I will update document later, right now priority for me is to figure out where I can enhance the incremental diff.

yaccz commented Feb 15, 2013

too big?

yac@deathstar % find -type f -exec du {} \;   
32  ./lib/shflags
8   ./lib/versions
4   ./lib/shlib
4   ./bin/gen_test_results.sh
4   ./bin/which
4   ./doc/RELEASE_NOTES-2.1.4.txt
16  ./doc/README.html
28  ./doc/LGPL-2.1
8   ./doc/rst2html.css
4   ./doc/RELEASE_NOTES-2.1.1.txt
4   ./doc/RELEASE_NOTES-2.1.3.txt
4   ./doc/RELEASE_NOTES-2.1.2.txt
4   ./doc/RELEASE_NOTES-2.1.5.txt
4   ./doc/RELEASE_NOTES-2.1.0.txt
4   ./doc/contributors.txt
20  ./doc/shunit2.txt
4   ./doc/TODO.txt
4   ./doc/coding_standards.txt
4   ./doc/RELEASE_NOTES-2.1.6.txt
36  ./doc/shunit2.html
8   ./doc/CHANGES-2.1.txt
8   ./doc/README.txt
4   ./doc/design_doc.txt
4   ./src/shunit2_test_standalone.sh
8   ./src/shunit2_test_misc.sh
8   ./src/shunit2_test_asserts.sh
8   ./src/shunit2_test_helpers
4   ./src/shunit2_test.sh
4   ./src/shunit2_test_failures.sh
32  ./src/shunit2
8   ./src/shunit2_test_macros.sh
4   ./examples/math_test.sh
4   ./examples/lineno_test.sh
4   ./examples/mkdir_test.sh
4   ./examples/equality_test.sh
4   ./examples/math.inc
4   ./examples/party_test.sh
--------------------------------------------------------------------------------
~/Downloads/shunit2-2.1.6  
yac@deathstar % find -type f -exec du {} \; | cut -f 1 | python -c 'import sys; print sum([int(x) for x in sys.stdin.readlines()])'
320

That's ridiculous

yaccz commented Feb 15, 2013

  1. It's not big, it's small
  2. it's reusable. Everyone who already knows shunit2 can just look into it with no overhead of learning YetAnotherLimitedTestRunnerThatOnlyThisFuckingProjectIsUsing.

yaccz commented Feb 15, 2013

WHY

Owner

ymattw commented Feb 16, 2013

That's ridiculous
It's not big, it's small

I need only 1 function, shunit2 is BIG for cdiff. Also shunit2 is actually to do unit test for shell scripts, cdiff is python, if you need unit test, use builtin unittest module or similar.

Everyone who already knows shunit2 can just look into it with no overhead of learning

For who doesn't already know shunit2, there's learning effort bigger than reading my simple script. I doubt how many people know it, because it's not smart to write shell scripts large enough and require unit tests.

YetAnotherLimitedTestRunnerThatOnlyThisFuckingProjectIsUsing

You use gentoo, is gentoo yet another Linux distribution? You use zsh, is zsh yet another shell?

Be nice, gentleman. Don't show angry face to people, it only makes yourself looks bad.

Learn some shell tips, try make your command line shorter.

yaccz commented Feb 16, 2013

k, I'm done with this. Looking forward to see your script that is not too big for cdiff.

ymattw closed this in 3b26195 Feb 16, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment