Skip to content

feature #2253 fix empty pytest logs #2485

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
Merged
merged 1 commit into from
Feb 7, 2024
Merged

feature #2253 fix empty pytest logs #2485

merged 1 commit into from
Feb 7, 2024

Conversation

John-Sharples
Copy link
Contributor

Pull Request Testing

  • Describe testing already performed for these changes:

    I have run the tests with and without this change to compare log file outputs. Appears to now write logged content to file.

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

    Run tests and check output

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? Yes

  • Do these changes include sufficient testing updates? Yes

  • Will this PR result in changes to the test suite? Yes

    If yes, describe the new output and/or changes to the existing output:

  • Please complete this pull request review by [Fill in date].

Pull Request Checklist

See the METplus Workflow for details.

  • Add any new Python packages to the METplus Components Python Requirements table.
  • Review the source issue metadata (required labels, projects, and milestone).
  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: @georgemccabe
    Select: METplus-Wrappers-6.0.0 Development
    Select: METplus-6.0.0
  • After submitting the PR, select the ⚙️ icon in the Development section of the right hand sidebar. Search for the issue that this PR will close and select it, if it is not already selected.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 7793212853

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 90.762%
Totals Coverage Status
Change from base Build 7700714846: 0.0%
Covered Lines: 8508
Relevant Lines: 9374
💛 - Coveralls

georgemccabe
georgemccabe approved these changes Feb 6, 2024
View reviewed changes
Copy link
Collaborator

@georgemccabe georgemccabe left a comment

Choose a reason for hiding this comment

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

Thank you for making this change, @John-Sharples. I tested by purposely breaking the code to cause a failed test. I see that the log files are now being populated. I noticed that the file name (and line number if I modify internal/tests/pytests/minimum_pytest.conf to include it) that is usually output for each log always says mock.py instead of the actual file that was logged.

(mock.py) DEBUG: Setting [config] PB2NC_MANDATORY to default value (True)
(mock.py) DEBUG: Setting [config] PB2NC_SKIP_IF_OUTPUT_EXISTS to default value (False)
(mock.py) DEBUG: Setting [config] USER_SHELL to default value (bash)
(mock.py) DEBUG: Setting [config] LOG_PB2NC_VERBOSITY to default value (2)
(mock.py) DEBUG: Setting [config] PB2NC_INPUT_DATATYPE to default value (empty string)
(mock.py) ERROR: (pb2nc_wrapper.py:158) force error to test

It does properly log the filename and line number for errors. If it is an easy fix to use the correct file name/number, that would be great, but it is not required. The ability to see all of the log lines in order is helpful for debugging purposes.

Thanks again for making this change!

@georgemccabe georgemccabe merged commit aa96daf into dtcenter:develop Feb 7, 2024
@John-Sharples John-Sharples deleted the feature_2253_fix_empty_log branch February 7, 2024 03:31
@John-Sharples
Copy link
Contributor Author

@georgemccabe

The mock simply calls config.logger with whatever args were passed to the mock, so it should just use the format specified by config. I had a quick dig to see where the formatting was coming from for the tests. It appears differnet tests use different configs, which are in several places within internal/tests/pytests/utils.

For example, changing this LOG_LINE_FORMAT changed the log output for tests using that config file.

@georgemccabe georgemccabe changed the title feature 2253 fix empty pytest logs feature #2253 fix empty pytest logs Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgemccabe georgemccabe georgemccabe approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@John-Sharples @coveralls @georgemccabe