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

Bringing us up to the latest pytest (3.4.0). Introduced a pytest.ini … #3995

Merged
merged 11 commits into from
Feb 16, 2018

Conversation

phoca2004
Copy link
Collaborator

@phoca2004 phoca2004 commented Feb 6, 2018

For reinstate manuscript test:
JIRA/TeamCity issue: https://teamcity.plos.org/teamcity/viewLog.html?buildId=148700&tab=buildResultsDiv&buildTypeId=Aperta_NoNoseIntegrationTestOnSfoCI#testNameId-2171890486949554082

What this PR does:

Bringing us up to the latest pytest (3.4.0). Introduced a pytest.ini to manage the configuration of logging options as the logging scheme in pytest changed fairly radically between 3.2.5 and 3.3.0 and again between 3.3.0 and 3.4.0. Removed old logging config in PlosPage to make it clear where we manage log options. Also relaxed some minor style validations, font-weight, that was causing a failure in test_reactivate_ms.py. Note you MUST update/upgrade pytest-sugar to v.0.9.1 for this change as the previous version (0.9.0) is incompatible with pytest 3.4.0.

Note that the scope of this PR was expanded to include a fix for an intermittent problem that is occurring for the figure task upload test. In that case we were comparing a uuencoded filename against a non-encoded filename and signalling a spurious error.

Notes

The incompatibility between pytest 3.4.0 and pytest-sugar is fixed in pytest-sugar 0.9.1. Please be sure you upgrade or sadness will ensue.

Code Review Tasks:

Reviewer tasks:

  • I read the code; it looks good
  • I ran the code (against a review environment, ci or other common environment)
  • If the PR changes code on which any other tests are dependent, I ran the dependent tests
  • I have found the tests to address all explicit and implicit AC or other test standards
  • I agree the author has fulfilled their tasks
  • All asserts output the failing attribute, ideally in context
  • All functions, classes have docstrings with all params and returns specified
  • Does not rely on dynamic, or excessively positional (more than two relations) locators
  • Does not rely on explicit sleeps except where absolutely necessary or dictated by the
    complexity of working around such use. Comment why when used.
  • Follows first PLOS style guidelines for Python, then PEP-8
  • Code is implemented in a Python 3 way
  • Teamcity PR designed to stabilize a failure in a test run should resolve ALL failures for the test file
  • Work implementing a new card addresses the points covered at: https://confluence.plos.org/confluence/display/TAHI/Considerations+when+creating+a+card+test+case
  • Code follows implementation guidance at: https://confluence.plos.org/confluence/display/FUNC/Implementing+your+python+end-to-end+tests

After the Code Review:

Reviewer tasks:

  • I have moved the ticket forward in JIRA

…to manage the configuration of logging options as the logging scheme in pytest changed fairly radically between 3.2.5 and 3.3.0 and again between 3.3.0 and 3.4.0. Removed old logging config in PlosPage to make it clear where we manage log options. Also relaxed some minor style validations, font-weight, that was causing a failure in test_reactivate_ms.py. Note you MUST uninstall pytest-sugar for this change as the latest version available as of right now (0.9.0) is incompatible with pytest 3.4.0.
@plos-ci-agent plos-ci-agent temporarily deployed to plos-ciagent-pr-3995 February 6, 2018 22:36 Inactive
@phoca2004 phoca2004 temporarily deployed to plos-ciagent-pr-3995 February 7, 2018 00:57 Inactive
patrickneve
patrickneve previously approved these changes Feb 8, 2018
Copy link
Collaborator

@patrickneve patrickneve left a comment

Choose a reason for hiding this comment

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

Looking good. The mismatched brackets make me feel better.

@phoca2004
Copy link
Collaborator Author

Thank you, Pat. Glad to make you feel better ;-)

I will point out, however, that we are sticklers, in Aperta for going through the various checks - indicated by the checkboxen in the description and checking them before signing off. Would you be so kind as to go through them?

@patrickneve patrickneve dismissed their stale review February 16, 2018 01:39

did not follow checklist

:return: True or False, if taskname is unknown.
"""
tasks = self._gets(self._task_headings)
time.sleep(3)

Choose a reason for hiding this comment

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

what is this sleep for?

#assert subhead.value_of_css_property('font-size') == '18px'
#assert subhead.value_of_css_property('line-height') == '25.7167px'
#assert subhead.value_of_css_property('color') == 'rgba(51, 51, 51, 1)'
Validating Main Heading

Choose a reason for hiding this comment

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

It's not clear from the docstring what this method is supposed to do.

# the page dialog that causes a failure. I feel dirty.
time.sleep(1)
# time.sleep(1)

Choose a reason for hiding this comment

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

could we remove this altogether?

line_height)
assert title.value_of_css_property('color') == color, \
'{0) is not equal to {1}'.format(title.value_of_css_property('color'), color)
'{0} is not equal to {1}'.format(title.value_of_css_property('color'), color)

Choose a reason for hiding this comment

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

👍

# Outputting the title allows us to validate update following conversion
manuscript_page.get_paper_short_doi_from_url()
title = manuscript_page.get_paper_title_from_page()
logging.info(u'Paper page title is: {0}'.format(title))

def test_core_validate_pp_submission_with_review_overlay(self):
def rest_core_validate_pp_submission_with_review_overlay(self):

Choose a reason for hiding this comment

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

we're disabling this test?

@achoe-PLOS
Copy link

@jgray-PLOS looks good overall; I just have a few comments.

@phoca2004
Copy link
Collaborator Author

Addressed all your comments @achoe-PLOS . Thank you for the thorough review.

Copy link

@achoe-PLOS achoe-PLOS left a comment

Choose a reason for hiding this comment

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

Looks good, @jgray-PLOS

@achoe-PLOS achoe-PLOS merged commit d25fef9 into master Feb 16, 2018
@achoe-PLOS achoe-PLOS deleted the pytest-logging-fixup branch February 16, 2018 21:57
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