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

Logging fixes #4106

Merged
merged 6 commits into from
Apr 29, 2021
Merged

Logging fixes #4106

merged 6 commits into from
Apr 29, 2021

Conversation

trexfeathers
Copy link
Contributor

🚀 Pull Request

Description

Improvements to all uses of logging in experimental.ugrid, following recent team discoveries.


Consult Iris pull request check list

@trexfeathers trexfeathers added Feature: UGRID Type: Feature Branch Highlight this for a feature branch labels Apr 22, 2021
@trexfeathers trexfeathers requested a review from pp-mo April 22, 2021 16:07
@trexfeathers trexfeathers marked this pull request as draft April 23, 2021 13:40
@trexfeathers trexfeathers marked this pull request as ready for review April 23, 2021 13:51
@pp-mo
Copy link
Member

pp-mo commented Apr 26, 2021

Useful improvements you've made here.
But on the testing side, I couldn't avoid thinking about how to improve this so I went digging and experimenting.
I came up with a possible way here : https://github.com/pp-mo/iris/pull/68/files

Don't want to undermine the work here, but I think the test changes are looking messy.
Would you consider incorporating into this PR the above extension to the assertLogs method ??
I do still have some concerns about the completeness of that aproach, though : see pp-mo#68

@trexfeathers
Copy link
Contributor Author

Don't want to undermine the work here, but I think the test changes are looking messy.

3cf6662 Does this help at all?

Would you consider incorporating into this PR the above extension to the assertLogs method ??

bfbed13 I can confirm from local testing that this addresses the issue you originally identified.

I do still have some concerns about the completeness of that aproach, though

I get what you're saying, however this area is new to us and we have addressed all issues we've encountered so far. No problem IMO.

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

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

Don't want to undermine the work here, but I think the test changes are looking messy.

3cf6662 Does this help at all?

I really meant the way we formerly had extra code to 'exercise' the logging before capturing + checking it.
So that is all squared away now 👍

Would you consider incorporating into this PR the above extension to the assertLogs method ??

bfbed13 I can confirm from local testing that this addresses the issue you originally identified.

Brilliant, thanks ! 💐

I do still have some concerns about the completeness of that aproach, though

I get what you're saying, however this area is new to us and we have addressed all issues
we've encountered so far. No problem IMO.

👍 I like your approach 😁

One thing that does still jar here, for me, is the repetition of the check_log method : we now have 4 copies (!)
I was very close to adding that (i.e. the regex check) into the new-version assertLogs, but I felt it was perhaps too much in one.
But what the hell, why not add an optional keyword like 'message_regex' + integrate the two?
The only snag, IMO, is that it only applies when you're expecting a single logged message. But naming/documentation can just allow for that.
See my suggestions (only partial changes : you will need to replace all the check_logs with assertLogs changes)

How does that way look to you @trexfeathers ?

@trexfeathers
Copy link
Contributor Author

How does that way look to you @trexfeathers ?

Very nice idea @pp-mo! 5a997d3 should take care of this - involved a fair amount of refactoring but the result is simpler, a little more akin to AssertRaisesRegex(), which is what I was originally after anyway.

@pp-mo pp-mo merged commit c088c33 into SciTools:mesh-data-model Apr 29, 2021
@pp-mo
Copy link
Member

pp-mo commented Apr 29, 2021

This all seems good to me now.
Thanks for sticking with it @trexfeathers

@trexfeathers trexfeathers deleted the logging_fixes branch August 31, 2021 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature: UGRID Type: Feature Branch Highlight this for a feature branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants