svalinn / dagmc_stats

Tool for calculating and reporting statistics about DAGMC models
BSD 3-Clause "New" or "Revised" License
1 stars 5 forks source link

Adds CI and better testing framework #60

Closed kkiesling closed 4 years ago

kkiesling commented 4 years ago

Description

The current test file was not being tested in CI and it turns out that many of the tests were failing. This is an attempt to clean it up and introduce CI.

This should address issue #30, #56, and #58.

Changes

Other Information

Some tests pass only because I had to comment out the assertions. Some of the function calls were incorrect and I was not sure what the tests were supposed to be doing. @yqin43 should look at those tests and make the appropriate fixes in a different PR.

kkiesling commented 4 years ago

Okay this is ready for review now. Note: I am not trying to make a comprehensive set of tests right now, but just setup the framework.

kkiesling commented 4 years ago

Thx for putting that in, nice work @kkiesling !! :)

why is this named dagmc_stats if it only rely on MOAB ?

I think you need better a name than scripts.... maybe dagmc_stats ?

maybe add a readme.md file to describe the h5m files in test... otherwise you will end up eventually with a test folder "à la" PyNE with a ton of files used for tests... but nobody knows what for what....

The dockerfile was named dagmc_stats just because this is the project it was made for. I agree, we should just make a MOAB-only depends docker image in Svalinn and I can start w/ that one. I tried to use the images already existing there but they don't have PyMOAB enabled. I can make new ones though.

And yes, agreed on the folder name. I was just thinking it should be renamed.

Good idea on the test folder README- I will add that too.

kkiesling commented 4 years ago

@bam241 or @piperlincoln - can you take a look at how I have the circleci config file setup now? I don't use a setup.py script. But I still want to be able to run tests locally in the test folder and also have them found in the docker image. And so far this is the only way I have been able to do that: set the PYTHONPATH and run pytest from inside the test directory on circleci.

I no longer have the PYTHONPATH set in the docker image for the dagmc_stats repo, but I do have it set for PyMOAB. I added the dagmc_stats pythonpath to the circleci config, but then PyMOAB wasn't found, so I had to add PyMOAB there as well.

bam241 commented 4 years ago

@bam241 or @piperlincoln - can you take a look at how I have the circleci config file setup now? I don't use a setup.py script. But I still want to be able to run tests locally in the test folder and also have them found in the docker image. And so far this is the only way I have been able to do that: set the PYTHONPATH and run pytest from inside the test directory on circleci.

I no longer have the PYTHONPATH set in the docker image for the dagmc_stats repo, but I do have it set for PyMOAB. I added the dagmc_stats pythonpath to the circleci config, but then PyMOAB wasn't found, so I had to add PyMOAB there as well.

I am sorry to insist, but I really think that appending to the PYTHONPATH is not a good and clean method. You will end-up with a PYTHONPATH pointing to half the folders in your computer.

I would rather se a very simple setup.py:

from setuptools import setup, find_packages

setup(name="dagmc_stats",  packages=find_packages())

and if you run a simple pip install -e . --user from your repo folder, you will be able to "run" locally. (this will create a tiny dagmc-stats.egg-link in you local PYTHON lib folder containing:

/path_to/dagmc_stats
.

this allows you to keep your PYTHONPATH clean

to remove the egg file you can: pip uninstall dagmc-stats

kkiesling commented 4 years ago

Ya ok- I think I understand what you all are saying now. This whole process of managing CI w/ or w/out a PYTHONPATH and a setup.py is all new to me. Let me try again. I will make a setup.py and try again.

kkiesling commented 4 years ago

okay- check the setup.py and config.yml that I have now?

Python 2 and 3 tests now run.

bam241 commented 4 years ago

This is looking good, one minor comment and I'll be ready to merge it :)

kkiesling commented 4 years ago

Thanks for all the input @bam241 and @piperlincoln! I think this is ready now.

Also- I had no idea you could use pip like that to install local packages. This is a game changer for so much of what I do.

bam241 commented 4 years ago

Thanks for all the input @bam241 and @piperlincoln! I think this is ready now.

Also- I had no idea you could use pip like that to install local packages. This is a game changer for so much of what I do.

I discovered it because of this. It is pretty cool :)

bam241 commented 4 years ago

lastly could you move your dockerfile into svalinn/dockerfile and push it to svalinn dockerhub ?

then change you circle.yml to point to it ?

the good thing is that because it is already pushed it should be super fast....

kkiesling commented 4 years ago

lastly could you move your dockerfile into svalinn/dockerfile and push it to svalinn dockerhub ?

then change you circle.yml to point to it ?

the good thing is that because it is already pushed it should be super fast....

done

bam241 commented 4 years ago

thx a ton! @kkiesling Merging !