Use fixture for the test #278

xiki-tempula · 2022-12-04T20:38:52Z

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 · 2022-12-04T20:46:00Z

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.

orbeckst

What's the advantage of putting all fixtures into conftest?

Are we re-using fixtures across multiple test files?

environment.yml

src/alchemlyb/tests/conftest.py

src/alchemlyb/tests/test_fep_estimators.py

xiki-tempula · 2022-12-05T17:29:26Z

@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

Looks ok in principle, we can move the fixtures to conftest. The important change is using the request.getfixture().

I want to have a closer look after PR #254 is merged — there's a lot of noise in this PR.

orbeckst · 2022-12-06T00:41:17Z

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

orbeckst

Looks ok. I have a few optional comments but no show-stoppers. Thank you for making some of the tests more efficient by re-using the new global fixtures.

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.

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 #280.

Please squash-merge when you're happy with it.

src/alchemlyb/tests/conftest.py

orbeckst · 2022-12-06T16:52:59Z

src/alchemlyb/tests/test_units.py

-    @pytest.fixture(scope='class')
-    def dhdl():
-        dataset = load_benzene()
-        dhdl = extract_dHdl(dataset['data']['Coulomb'][0], 310)
-        return dhdl
-
-    @staticmethod
-    @pytest.fixture(scope='class')
-    def u_nk():
-        dataset = load_benzene()
-        u_nk = extract_u_nk(dataset['data']['Coulomb'][0], 310)
-        return u_nk
-
-    @pytest.mark.parametrize('func,fixture_in',
-                             [(dhdl2series, 'dhdl'),
-                              (u_nk2series, 'u_nk'),
-                              (decorrelate_u_nk, 'u_nk'),
-                              (decorrelate_dhdl, 'dhdl'),
-                              (slicing, 'dhdl'),
-                              (statistical_inefficiency, 'dhdl'),
-                              (equilibrium_detection, 'dhdl')])


Putting these into conftest.py seems a bit messy because they are named so generically that it's not clear when you'd want to use them. With the code close by, it's a little bit clearer. Maybe if they were called gmx_dHdl and gmx_u_nk?

Sorry these are all mess made by black.

orbeckst · 2022-12-06T17:26:09Z

src/alchemlyb/tests/parsing/test_namd.py


-    filenames = load_idws()['data']['forward']
+    filenames = load_idws()["data"]["forward"]


Maybe make load_idws a fixture, too, for consistency?

Sorry, this is another black thing. Now I fixed the diff, you could see that these are all outdated.

orbeckst · 2022-12-06T17:34:14Z

src/alchemlyb/tests/parsing/test_gmx.py

-    """Test that dHdl has the correct form when extracted from files.
-
-    """
+    """Test that dHdl has the correct form when extracted from files."""
    dataset = load_benzene()


This test file has a lot of instances of dataset = load_something() that could eventually be turned into fixtures. It makes no big difference for performance, I think, but it would make our approach more uniform.

@pytest.fixture def gmx_benzene_data(): return gmx.load_benzene() def test_dHdl(gmx_benzene_data): ...

However, no need to do it in this PR, just for the future.

So the logic is that the conftest will do and will only do the parsing, additional operations like concat will be done at the local level. I have added the description to the top of conftest.

orbeckst · 2022-12-06T17:43:13Z

Please resolve conflicts (after PR #280)

src/alchemlyb/tests/test_preprocessing.py

xiki-tempula · 2022-12-06T21:09:01Z

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 #280.

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

orbeckst · 2022-12-06T21:19:17Z

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

LGTM, now that I can see how you use the global fixtures, it makes sense and leads to cleaner tests. Well done.

orbeckst · 2022-12-06T21:28:29Z

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

orbeckst · 2022-12-06T21:32:15Z

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

xiki-tempula added 17 commits August 14, 2022 20:56

remove iloc

f1a8ae2

update loc

afc54b8

Merge branch 'master' into feat_iloc

c623b14

update

3fd1005

update

7609dc2

ifx test

50bf8aa

Merge branch 'master' into feat_iloc

e049ccb

update

e4c0a4d

update

6839b8b

update

6d8ec6b

update

2a2d11f

update

91a3928

update

0d81a47

update

1afd2b9

update

1ab2c67

update

c879924

update

ded5d1b

Merge branch 'feat_RTD' into feat_fixture

69155b6

xiki-tempula requested a review from orbeckst December 4, 2022 22:03

orbeckst reviewed Dec 5, 2022

View reviewed changes

environment.yml Outdated Show resolved Hide resolved

src/alchemlyb/tests/conftest.py Show resolved Hide resolved

src/alchemlyb/tests/test_fep_estimators.py Show resolved Hide resolved

xiki-tempula added 2 commits December 5, 2022 17:10

Merge branch 'master' into feat_fixture

68b7be4

update

06c1de0

xiki-tempula mentioned this pull request Dec 5, 2022

patch release 1.0.1 #279

Closed

orbeckst requested changes Dec 5, 2022

View reviewed changes

xiki-tempula force-pushed the feat_fixture branch from fdf9e1c to 06c1de0 Compare December 6, 2022 10:40

black

e950af2

orbeckst approved these changes Dec 6, 2022

View reviewed changes

orbeckst assigned xiki-tempula Dec 6, 2022

xiki-tempula force-pushed the feat_fixture branch from 996d508 to e950af2 Compare December 6, 2022 20:57

xiki-tempula added 2 commits December 6, 2022 20:59

Merge branch 'feat_fixture'

725efb0

Merge branch 'master' into feat_fixture

8c6bb24

xiki-tempula commented Dec 6, 2022

View reviewed changes

src/alchemlyb/tests/test_preprocessing.py Show resolved Hide resolved

update

524efe1

update

2809915

orbeckst approved these changes Dec 6, 2022

View reviewed changes

orbeckst self-assigned this Dec 6, 2022

orbeckst merged commit fc04c01 into alchemistry:master Dec 6, 2022

xiki-tempula deleted the feat_fixture branch December 6, 2022 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fixture for the test #278

Use fixture for the test #278

xiki-tempula commented Dec 4, 2022 •

edited

Loading

codecov bot commented Dec 4, 2022 •

edited

Loading

orbeckst left a comment

xiki-tempula commented Dec 5, 2022

orbeckst left a comment

orbeckst commented Dec 6, 2022

orbeckst left a comment

orbeckst Dec 6, 2022

xiki-tempula Dec 6, 2022

orbeckst Dec 6, 2022

xiki-tempula Dec 6, 2022

orbeckst Dec 6, 2022

xiki-tempula Dec 6, 2022 •

edited

Loading

orbeckst commented Dec 6, 2022

xiki-tempula commented Dec 6, 2022

orbeckst commented Dec 6, 2022

orbeckst left a comment

orbeckst commented Dec 6, 2022

orbeckst commented Dec 6, 2022


		filenames = load_idws()['data']['forward']
		filenames = load_idws()["data"]["forward"]

Use fixture for the test #278

Use fixture for the test #278

Conversation

xiki-tempula commented Dec 4, 2022 • edited Loading

codecov bot commented Dec 4, 2022 • edited Loading

Codecov Report

orbeckst left a comment

Choose a reason for hiding this comment

xiki-tempula commented Dec 5, 2022

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Dec 6, 2022

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Dec 6, 2022

Choose a reason for hiding this comment

xiki-tempula Dec 6, 2022

Choose a reason for hiding this comment

orbeckst Dec 6, 2022

Choose a reason for hiding this comment

xiki-tempula Dec 6, 2022

Choose a reason for hiding this comment

orbeckst Dec 6, 2022

Choose a reason for hiding this comment

xiki-tempula Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

orbeckst commented Dec 6, 2022

xiki-tempula commented Dec 6, 2022

orbeckst commented Dec 6, 2022

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Dec 6, 2022

orbeckst commented Dec 6, 2022

xiki-tempula commented Dec 4, 2022 •

edited

Loading

codecov bot commented Dec 4, 2022 •

edited

Loading

xiki-tempula Dec 6, 2022 •

edited

Loading