Skip to content

Add unit tests to chemkinTest#952

Merged
KEHANG merged 4 commits intomasterfrom
chemkinTest
Mar 21, 2017
Merged

Add unit tests to chemkinTest#952
KEHANG merged 4 commits intomasterfrom
chemkinTest

Conversation

@mliu49
Copy link
Copy Markdown
Contributor

@mliu49 mliu49 commented Mar 18, 2017

I started out trying to write tests for each untested line, but I realized that it wasn't really helping. A major untested part was writing chemkin files, so I added some additional checks to existing tests. The end result was only +6% coverage within chemkin.py. To increase coverage more, we need to test reading and writing the other kinetics formats besides Arrhenius and Chebyshev.

@mention-bot
Copy link
Copy Markdown

@mliu49, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvandewiele, @connie and @jwallen to be potential reviewers.

@KEHANG
Copy link
Copy Markdown
Member

KEHANG commented Mar 21, 2017

@mliu49 not sure what mock is doing here. Can you explain why we need add it as a new RMG dependency? or how you recommend using it in the future?

@mliu49
Copy link
Copy Markdown
Contributor Author

mliu49 commented Mar 21, 2017

mock enables creation of fake instances of python modules. In this case, mock is used to create a fake logging module to intercept calls to logging.warning. This allows unit testing that specific logging statements are actually created.

In this case, the unit tests I added aren't particularly important, but I had written them in my initial attempt to cover line-by-line. I quickly realized that that wasn't a particularly good way to go about it, but I kept the tests anyway.

If you don't think those tests are worth keeping, then I'm fine with not adding them. However, I think mock does have future value in unit testing.

Edit: FYI, mock was converted to a standard module in python 3.

@rwest
Copy link
Copy Markdown
Member

rwest commented Mar 21, 2017

I like it. But perhaps add a few docstrings or comments in the unit test to help others understand what on earth is going on? To someone who's not worked with mock before, it's not particularly obvious. And it might be something we want to replicate elsewhere.

@mliu49
Copy link
Copy Markdown
Contributor Author

mliu49 commented Mar 21, 2017

I will add more info about mock to the docstrings.

@KEHANG
Copy link
Copy Markdown
Member

KEHANG commented Mar 21, 2017

@mliu49 ready to merge?

@KEHANG KEHANG merged commit 17ff242 into master Mar 21, 2017
@KEHANG KEHANG deleted the chemkinTest branch March 21, 2017 21:16
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.

4 participants