Niemeyer-Research-Group / pyMARS

Python-based (chemical kinetic) Model Automatic Reduction Software
https://niemeyer-research-group.github.io/pyMARS/
MIT License
61 stars 46 forks source link

Automated testing #16

Closed xMestas closed 6 years ago

xMestas commented 6 years ago

Fixes bug in the PFA method, and adds integration tests for DRGEPSA and PFA

xMestas commented 6 years ago

The files expected to be produced by the reduction were generated and placed in the test directory. For each method, the test will execute the reduction to produce a reduced solution, and read in the expected solution from the expected file. Attributes of the final solution and the expected final solution are then compared for equivalence.

kyleniemeyer commented 6 years ago

Rather than generate files in the test directory, can we instead have the code generate a temporary directory and use that? That way the module directory won't get cluttered up with files that aren't necessary after the tests complete.

Here's an example, taken from http://stackoverflow.com/a/22726782/1569494:

try:
    from tempfile import TemporaryDirectory
except ImportError:
    from contextlib import contextmanager
    import shutil
    import tempfile
    import errno

    @contextmanager
    def TemporaryDirectory():
        name = tempfile.mkdtemp()
        try:
            yield name
        finally:
            try:
                shutil.rmtree(name)
            except OSError as e:
                # Reraise unless ENOENT: No such file or directory
                # (ok if directory has already been deleted)
                if e.errno != errno.ENOENT:
                    raise

and then used with

with TemporaryDirectory() as temp_dir:
    ... # do stuff in temp_dir
kyleniemeyer commented 6 years ago

This is the method I used for tests in PyTeCK

xMestas commented 6 years ago

Or the files generated by the test could just be deleted, since they aren't needed. Would that work?

kyleniemeyer commented 6 years ago

Personally I think it's neater (and safer) to create a system temporary folder that will automatically be removed, rather than actually delete files—less room accidental deletion and leaving things behind.

xMestas commented 6 years ago

The problem is that the function being called in the test generates the file in the cwd. Should I also make a change where it generates the file on a given path? And then change the test to make a temp directory and then generate the file in the temp directory?

kyleniemeyer commented 6 years ago

Yes, I think the path should be changeable (as in, should be a function argument). And then have the test make a temp directory and make the file there.

On Oct 22, 2018, at 1:56 PM, Phillip Mestas notifications@github.com wrote:

The problem is that the function being called in the test generates the file in the cwd. Should I also make a change where it generates the file on a given path? And then change the test to make a temp directory and then generate the file in the temp directory?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

xMestas commented 6 years ago

What about moving the function call that writes the file up to the pymars function? No need to call it in every method when it can just be called once at the end. This will stop the files from being generated by the test too, removing the need for a temp directory. The path to which the file is written can still be added as an argument to just the pymars function and not the function that does the actual reduction.

xMestas commented 6 years ago

Pushed the changes stated above to the pull request

kyleniemeyer commented 6 years ago

Moving that function up seems fine, but regardless I think any creation of files should be done in one of these temporary folders—it's just cleaner and safer than anything else.

xMestas commented 6 years ago

Yup, the output files are no longer created by the test though, and those files are meant to be given to the user, so they shouldn't be deleted. The hdf5 files could be stored in a tmp dir and deleted after, but they may be useful to hold on to for reuse as you said before. I have that recorded as an issue to move them to their own directory.

kyleniemeyer commented 6 years ago

Looks like there is a merge conflict now?

xMestas commented 6 years ago

Merge conflict should be resolved now.