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

Use runtest library for testing; fixes #28, fixes #80 #82

Merged
merged 28 commits into from
Dec 1, 2017
Merged

Use runtest library for testing; fixes #28, fixes #80 #82

merged 28 commits into from
Dec 1, 2017

Conversation

bast
Copy link
Member

@bast bast commented Nov 30, 2017

Here we switch to https://github.com/bast/runtest for testing.

Motivation:

  • Less code (import well calibrated scripts instead of writing them)
  • More control over numerical tolerance
  • More clear separation between inputs and reference outputs
  • More flexibility for extracting numbers and comparing with numerical tolerance
  • When running make test no temporary files are generated outside build directory
  • Easier to create new tests
  • More portability (Python does the testing)
  • More fine grained tests (we see more clearly where things break)

@bast bast requested a review from heikef November 30, 2017 18:38
@bast
Copy link
Member Author

bast commented Nov 30, 2017

This is quite a monster change set so take time with review. Also @mariavd should have a say since this PR nukes her scripts. Sorry :-) but hopefully this change makes sense.

@bast
Copy link
Member Author

bast commented Nov 30, 2017

OK please wait with integrating it - some tests fail on Travis so I need to look at them. But we can still discuss the code changes until I restore the tests also on Travis. I think this is due to numerical tolerance but I need to verify.

@mariavd
Copy link
Contributor

mariavd commented Nov 30, 2017 via email

@heikef
Copy link
Contributor

heikef commented Nov 30, 2017

Thanks for the initiative, Radovan! Perhaps we should first clone your feature branch and test?
Concerning the numerics, it would be good to have an agreement within 3-4 digits in the current strength susceptibility, when possible.

@bast
Copy link
Member Author

bast commented Nov 30, 2017

Done some more debugging. The problem is not tolerance. There seems to be some subtle problem since the output called "stdout" cannot be found on Travis.

@bast
Copy link
Member Author

bast commented Nov 30, 2017

What I personally find a bit annoying is that GIMIC writes output to stdout instead of writing it into a file. This complicates my testing and is I think also the reason for issues on Travis.

@bast
Copy link
Member Author

bast commented Dec 1, 2017

And I forgot to answer a question: of course you are welcome to try out my fork/branch directly. I trust that all works fine. Only on Travis it doesn't and I need to figure out why.

@heikef
Copy link
Contributor

heikef commented Dec 1, 2017

Thanks Radovan. I think the stdout out thing is useful for the dryrun option. It is convenient for a user.

@heikef
Copy link
Contributor

heikef commented Dec 1, 2017

Travis tests are important for us. We can remove the stdout and put everything into the gimic.out file. Would that be a solution? To me it sounds simple and fast.

@mariavd
Copy link
Contributor

mariavd commented Dec 1, 2017

I completely agree.

@heikef
Copy link
Contributor

heikef commented Dec 1, 2017

Perhaps we should open a scripts / input issue? @mariavd There we could collect all the pros and cons. I remember you had some ideas / requests and comments. See #84 issue.

@bast
Copy link
Member Author

bast commented Dec 1, 2017

OK I will continue debugging since I still don't fully understand why it fails. I will write as soon as I know more.

@bast
Copy link
Member Author

bast commented Dec 1, 2017

Fixed a couple of bugs. Now it's all good and ready to go but of course you still may want to review it.

@bast
Copy link
Member Author

bast commented Dec 1, 2017

Crap - now it's conflicting. Will submit a fix in a moment.

@heikef
Copy link
Contributor

heikef commented Dec 1, 2017

As far as I can see it things are fine. It is more important to proceed instead of investing too much time in Travis. Testing can always be changed or adapted. We have more important issues on the desk.

Conflicts:
    src/CMakeLists.txt
@bast
Copy link
Member Author

bast commented Dec 1, 2017

It works now on Travis. Fixed the conflict. It should be all fine.

@heikef
Copy link
Contributor

heikef commented Dec 1, 2017

Perfect! Thank you!

@bast
Copy link
Member Author

bast commented Dec 1, 2017

And about the issues on the desk: they are impossible to do if we don't have a good testing framework - this is why I spend so much energy into getting the testing and benchmarks in shape.

@heikef heikef merged commit 53eb136 into qmcurrents:master Dec 1, 2017
@heikef
Copy link
Contributor

heikef commented Dec 1, 2017

I know Radovan. My point was when testing works we should not optimize the testing but rather the code. Thanks for your efforts!

@bast bast deleted the radovan/runtest branch December 1, 2017 11:31
@mariavd
Copy link
Contributor

mariavd commented Dec 7, 2017

Is the new testing supposed to be used as $ make test?

This is what I got on a cluster:

$ make test
Running tests...
Test project /home/mariavd/kevin-test/gimic/build
    Start 1: benzene/integration
1/9 Test #1: benzene/integration ..............***Failed    0.03 sec
    Start 2: benzene/bond-integration
2/9 Test #2: benzene/bond-integration .........***Failed    0.03 sec
    Start 3: benzene/vectors
3/9 Test #3: benzene/vectors ..................***Failed    0.03 sec
    Start 4: benzene/2d
4/9 Test #4: benzene/2d .......................***Failed    0.03 sec
    Start 5: benzene/3d
5/9 Test #5: benzene/3d .......................***Failed    0.03 sec
    Start 6: benzene/keyword-magnet
6/9 Test #6: benzene/keyword-magnet ...........***Failed    0.03 sec
    Start 7: benzene/3d-keyword-magnet
7/9 Test #7: benzene/3d-keyword-magnet ........***Failed    0.03 sec
    Start 8: benzene/2d-keyword-magnet
8/9 Test #8: benzene/2d-keyword-magnet ........***Failed    0.03 sec
    Start 9: c4h4/integration
9/9 Test #9: c4h4/integration .................***Failed    0.03 sec

0% tests passed, 9 tests failed out of 9

Total Test time (real) =   0.27 sec

The following tests FAILED:
	  1 - benzene/integration (Failed)
	  2 - benzene/bond-integration (Failed)
	  3 - benzene/vectors (Failed)
	  4 - benzene/2d (Failed)
	  5 - benzene/3d (Failed)
	  6 - benzene/keyword-magnet (Failed)
	  7 - benzene/3d-keyword-magnet (Failed)
	  8 - benzene/2d-keyword-magnet (Failed)
	  9 - c4h4/integration (Failed)
Errors while running CTest
make: *** [test] Error 8

Going to the test directory and calling gimic with the input files works fine.

@bast
Copy link
Member Author

bast commented Dec 7, 2017

Yes. It is expected to work with either make test or ctest. Even better: ctest -jN where N is number of cores. Have a look why they failed under build/Testing. Also try to run the tests from their local directories using the test script.

@mariavd
Copy link
Contributor

mariavd commented Dec 7, 2017

Right, so runtest is missing.

    from runtest import version_info, get_filter, cli, run
ImportError: No module named runtest
<end of output>

How can I add it without messing up the system python (and not go through the mess of virtual environments, they are not a solution in my opinion)? Or should I use Anaconda?

@bast
Copy link
Member Author

bast commented Dec 7, 2017

You need to install it :-) I think you have an outdated virtual environment. See requirements.txt.

@bast
Copy link
Member Author

bast commented Dec 7, 2017

Why is venv a mess? IMO system python is almost always not the solution.

@bast
Copy link
Member Author

bast commented Dec 7, 2017

Next time let us please not discuss on a closed PR - the risk is that I miss such a discussion. Next time please new issue. But now we continue here until this is resolved. Venv is the way to go. But of course I want to help with problems concerning venvs.

@mariavd
Copy link
Contributor

mariavd commented Dec 7, 2017

@bast Alright, I will start a new issue next time. My illiteracy in Python is speaking here. What I mean to ask is, is it safe to use pip install runtest for the system python? And when is that not a good idea? Not long ago I updated Cython for the system python on our cluster using /opt/rh/python27/root/usr/bin/python /opt/rh/python27/root/usr/bin/pip install --upgrade pip, I was warned not to use pip for it any more because it ended up somehow causing problems with GLIBC. How is that possible is another question that I won't even ask.

@bast
Copy link
Member Author

bast commented Dec 7, 2017

It's probably safe but I would never install any Python packages system-wide with the only exception of installing virtualenv. Everything else I always install into a venv. What is not clear or not working about venv?

@mariavd
Copy link
Contributor

mariavd commented Dec 7, 2017

@bast Thanks, I sorted out the virtual environment and all tests passed.

@bast
Copy link
Member Author

bast commented Dec 7, 2017

Nice!

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.

3 participants