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.

Sign up for GitHub

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

Jump to bottom

Pytest Style: test_log.py #1467

Merged
merged 6 commits into from
Jul 13, 2017
Merged

Pytest Style: test_log.py #1467

merged 6 commits into from
Jul 13, 2017

Conversation

utkbansal
Copy link
Member

Fixes #

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

orbeckst
orbeckst requested changes Jul 11, 2017
View reviewed changes
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

only 1 change, otherwise good

testsuite/MDAnalysisTests/utils/test_log.py Outdated


def _assert_in(output, string):
assert_(string in output,
Copy link
Member

Choose a reason for hiding this comment

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

use plain pytest assert:

assert string in output, "Output '{0}' does not match required format '{1}'.".format(output.replace('\r', '\\r'), string.replace('\r', '\\r')))

@utkbansal
Copy link
Member Author

@kain88-de Review please.

jbarnoud
jbarnoud requested changes Jul 11, 2017
View reviewed changes
Copy link
Contributor

@jbarnoud jbarnoud left a comment

Choose a reason for hiding this comment

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

All occurrences of assert_raises should be replaced by the pytest equivalent. The deactivated assert_warn at the end of the file should be reactivated and moved to the pytest idiom.

The tests about the progress bar can probably be refactored as a single parametrized function.

testsuite/MDAnalysisTests/utils/test_log.py Outdated
from MDAnalysis.lib.log import _set_verbose
from numpy.testing import assert_, assert_equal, assert_raises
Copy link
Contributor

Choose a reason for hiding this comment

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

assert_ and assert_raises should not be used.

testsuite/MDAnalysisTests/utils/test_log.py Outdated


def test_default_ProgressMeter(buffer, n=101, interval=10):
format = "Step {step:5d}/{numsteps} [{percentage:5.1f}%]"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing to do with pytest, but to avoid confusion could you rename this variable template? Same goes for all subsequent occurrences.

kain88-de
kain88-de previously approved these changes Jul 11, 2017
View reviewed changes
kain88-de
kain88-de reviewed Jul 11, 2017
View reviewed changes
testsuite/MDAnalysisTests/utils/test_log.py Outdated
buffer.seek(0)
output = "".join(buffer.readlines())
_assert_in(output, (format + '\n').format(**{'step': 1, 'numsteps': n, 'percentage': 100./n}))
_assert_in(output, (format + '\n').format(**{'step': n, 'numsteps': n, 'percentage': 100.}))


def test__set_verbose():
Copy link
Member

Choose a reason for hiding this comment

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

the below test screams for a mark.parametrize

@kain88-de kain88-de dismissed their stale review July 11, 2017 17:18

Johnathan has some good points

@utkbansal
Copy link
Member Author

Someone want's to verify why actually test_warnings__set_verbose fails?

jbarnoud
jbarnoud requested changes Jul 12, 2017
View reviewed changes
Copy link
Contributor

@jbarnoud jbarnoud left a comment

Choose a reason for hiding this comment

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

I do not know why the warning tests fail. I check and the function does issue the warning... I'll try to have a deeper look tonight.

testsuite/MDAnalysisTests/utils/test_log.py
_assert_in(output, (template + '\n').format(**{'step': step, 'numsteps': n, 'percentage': percentage}))


class TestSetVerbose(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there are two methods that basically are testing the same log think. I thought it makes sense to group them. No?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense indeed.

testsuite/MDAnalysisTests/utils/test_log.py
assert string in output, "Output '{0}' does not match required format '{1}'.".format(output.replace('\r', '\\r'), string.replace('\r', '\\r'))


def test_default_ProgressMeter(buffer, n=101, interval=10):
Copy link
Contributor

Choose a reason for hiding this comment

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

The test_*_ProgressMeter functions are basicaly all doing the same thing. They should be parametrized.

testsuite/MDAnalysisTests/utils/test_log.py Outdated

@pytest.fixture()
def buffer():
Copy link
Contributor

Choose a reason for hiding this comment

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

Good that you renamed buf to buffer. It was a terrible name, my bad.

@jbarnoud
Copy link
Contributor

Someone want's to verify why actually test_warnings__set_verbose fails?

This seems to be the answer: https://docs.pytest.org/en/latest/warnings.html#ensuring-function-triggers

@utkbansal
Copy link
Member Author

utkbansal commented Jul 12, 2017 •
edited
Loading

@jbarnoud @kain88-de @richardjgowers

  • test_not_dynamic_ProgressMeter has a different _assert_in statement - it doesn't have \r so that will remain seperate.

  • test_default_ProgressMeter has

pm = MDAnalysis.lib.log.ProgressMeter(n, interval=interval)
        for frame in range(n):
            pm.echo(frame)

remaining two have (test_custom_ProgressMeter & test_legacy_ProgressMeter)

pm = MDAnalysis.lib.log.ProgressMeter(n, interval=interval,
                                              format=template, offset=1)
        for frame in range(n):
            rmsd = 0.02 * frame * (n+1)/float(n)  # n+1/n correction for 0-based frame vs 1-based counting
            pm.echo(frame, rmsd=rmsd)

so there is a change in rmsd hence test_default_ProgressMeter can't be merged either.

In the end I think I can only merge test_custom_ProgressMeter & test_legacy_ProgressMeter into one and parametrize them. Am I correct?

@jbarnoud
Copy link
Contributor

I still think there is a way to avoid the redundancy, but it may induce some complexity we do not want in the tests. Do not loose time on this.

@utkbansal
Copy link
Member Author

utkbansal commented Jul 12, 2017 •
edited
Loading

So what do we do? Merge only the two cases? Or merge the PR as is?

@jbarnoud
Copy link
Contributor

If you fix the warning tests, I'll merged as it.

@utkbansal
Copy link
Member Author

@jbarnoud Added these lines to the test

warnings.simplefilter('always')
warnings.warn("deprecated", DeprecationWarning)

Now pytest utils/test_log.py::TestSetVerbose::test_warnings__set_verbose passes but pytest utils/test_log.py::TestSetVerbose fails.

@jbarnoud
Copy link
Contributor

Why not using pytest.deprecated_call?

@utkbansal
Copy link
Member Author

@jbarnoud This is done I think.

@jbarnoud jbarnoud merged commit ae6fca7 into MDAnalysis:develop Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kain88-de kain88-de kain88-de left review comments

@orbeckst orbeckst orbeckst approved these changes

@jbarnoud jbarnoud jbarnoud approved these changes

Assignees
No one assigned
Labels
testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@utkbansal @jbarnoud @orbeckst @kain88-de