alchemistry / alchemlyb

the simple alchemistry library
https://alchemlyb.readthedocs.io
BSD 3-Clause "New" or "Revised" License
189 stars 49 forks source link

Use fixture for the test #278

Closed xiki-tempula closed 1 year ago

xiki-tempula commented 1 year ago

Fix #206 Merge after #254 This PR makes all the parsing for the tests done at the fixture level. I have not touched the parsing part as the test itself is to check the parsing. I have also not touched the ABFE part as I want to refactor the test a bit in another PR, where I might remove or change some of the tests. This PR didn't change the tests only had them load data from the fixture

codecov[bot] commented 1 year ago

Codecov Report

Merging #278 (2809915) into master (0095be2) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #278   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          26       26           
  Lines        1761     1763    +2     
  Branches      379      380    +1     
=======================================
+ Hits         1738     1740    +2     
  Misses          3        3           
  Partials       20       20           
Impacted Files Coverage Δ
src/alchemlyb/__init__.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

xiki-tempula commented 1 year ago

@orbeckst

What's the advantage of putting all fixtures into conftest? Are we re-using fixtures across multiple test files?

Yes, almost all of the tests start from a pandas dataframe and they come from a parsing action, which is now dealt at fixture level.

orbeckst commented 1 year ago

Please resolve conflicts & then I'll have a final look. Thanks!

orbeckst commented 1 year ago

Please resolve conflicts (after PR #280)

xiki-tempula commented 1 year ago

Sorry about all the black fuss. I thought blackfy this PR would make the diff it more clear but it seems that it has been made noisier.

In some cases, it might be a bit confusing when global fixtures with generic names such u_nk and dHdl are used. Previously it was clearer when these fixtures where local to the class or module and one could directly see their code. I'd either keep them local or make their names more explicit.

So the logic is that at conftest the fixture are all named in a descriptive format like gmx_water_particle_without_energy_dHdl, then locally, the fixture can be named as u_nk and dHdl to make the individual tests more clear.

As a sidenote, it was difficult and time-consuming to review the PR with all the black code formatting changes. It would have been better if you had waited with the reformatting with PR https://github.com/alchemistry/alchemlyb/pull/280.

Sorry for the confusion, I guess I made a mess of the black thing but the diff should be clear now.

orbeckst commented 1 year ago

So the logic is that at conftest the fixture are all named in a descriptive format like gmx_water_particle_without_energy_dHdl, then locally, the fixture can be named as u_nk and dHdl to make the individual tests more clear.

Oh, I didn't pick up on any local renaming. That's a reasonable approach.

orbeckst commented 1 year ago

I am going to merge it and if there's anything else we can do it in a later PR.

orbeckst commented 1 year ago

Bonus: a net loss of almost 100 lines of code 👍