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

Fix filename bug #54

Merged
merged 3 commits into from
Mar 18, 2018
Merged

Fix filename bug #54

merged 3 commits into from
Mar 18, 2018

Conversation

mimischi
Copy link
Contributor

@mimischi mimischi commented Mar 15, 2018

Fixes #53.

Changes made in this pull request:

  • Change the way we handle filenames.

PR Checklist

  • Added unit tests
  • Added changelog fragment in ./changelog/ (more information)?
  • Issue raised/referenced?

@codecov
Copy link

codecov bot commented Mar 15, 2018

Codecov Report

Merging #54 into master will decrease coverage by 0.21%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   87.27%   87.06%   -0.22%     
==========================================
  Files          11       11              
  Lines         448      456       +8     
==========================================
+ Hits          391      397       +6     
- Misses         57       59       +2
Impacted Files Coverage Δ
mdbenchmark/generate.py 95.83% <100%> (ø) ⬆️
mdbenchmark/mdengines/gromacs.py 100% <100%> (ø) ⬆️
mdbenchmark/mdengines/namd.py 97.53% <71.42%> (-2.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f88f074...9ecfd9f. Read the comment docs.

gpu=gpu,
module=m,
full_filename=full_filename,
name=engine_filename,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name isn't used anymore and it must have been defined.

@mimischi
Copy link
Contributor Author

I cannot wrap my head around that. My local tests fail at this line, while Travis-CI does not seem to have any problems. I've cleared all caches already using make clean.

@mimischi
Copy link
Contributor Author

Just realized what is causing the problem: for testing purposes I created md.namd, md.psf and md.pdb inside my mdbenchmark folder. Somehow pytest picks these up during testing and errors out. Deleting those resolves the issues.

Is something is wrong with our pytest setup?

* Test the file name check.
* Add another test to check the bench.job variable replacement.
@mimischi
Copy link
Contributor Author

@kain88-de This is finished. I'd like to finish #49 before we release a new version.

@kain88-de
Copy link
Contributor

Somehow pytest picks these up during testing and errors out. Deleting those resolves the issues.

I don't know what files you had exactly so I can't say. To my knowledge, there isn't anything wrong. Also, test files should best be generated in a folder outside of the source code.

@@ -0,0 +1 @@
The provided filename is now correctly parsed and provided to the job templates.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better 'Fixed edge-case in input filename parsing.'

It's not like it wasn't parsed correctly before. We just missed a specific edge case.

@mimischi
Copy link
Contributor Author

I don't know what files you had exactly so I can't say. To my knowledge, there isn't anything wrong. Also, test files should best be generated in a folder outside of the source code.

touch md.namd, touch md.psf and touch md.pdb inside the root folder. Then run pytest inside of that.

@kain88-de
Copy link
Contributor

you invoke the CLI interface from the folder where you call pytest. To avoid this behavior you need to ensure that the cli invocation happens in a temporary folder. It might be time to refactor the CLI testing functions.

@kain88-de kain88-de merged commit bfee8f3 into master Mar 18, 2018
@kain88-de kain88-de deleted the fix-filename branch March 18, 2018 12:39
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.

2 participants