You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/12/22 10:57:13 UTC
[GitHub] [solr] mariemat opened a new pull request #468: ensures that test code does not depend on log level
mariemat opened a new pull request #468:
URL: https://github.com/apache/solr/pull/468
There is no associated JIRA.
This is a simple test code cleanup.
Current code will not wait if log level is not at INFO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on pull request #468: ensures that test code does not depend on log level
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #468:
URL: https://github.com/apache/solr/pull/468#issuecomment-999545613
Probably no need for a JIRA. And executing logic inside a logger.info() call sounds like a bug, since if you run tests on WARN level it will be have differently. Wrapping logging in isXxxEnabled() is normally done for TRACE and DEBUG for performance reasons, so not using it for INFO is ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] mariemat commented on pull request #468: ensures that test code does not depend on log level
Posted by GitBox <gi...@apache.org>.
mariemat commented on pull request #468:
URL: https://github.com/apache/solr/pull/468#issuecomment-999677006
> Took me a second to understand this PR... Is the `waitForWatcher` critical for the path of this test? And since it's wrapped in the logging check, then it wouldn't be run?
Yes, this is exactly the issue I want to fix to make the test morr robust
> I wonder if the logic should be to have the waitForWatcher to happen, but then to wrap the logging in the if statement? Since that is the pattern that we use everywhere?
Other logging at info level in that test was not in if statements, Iso I removed it from that only place in that file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #468: ensures that test code does not depend on log level
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #468:
URL: https://github.com/apache/solr/pull/468#issuecomment-999540630
Took me a second to understand this PR... Is the `waitForWatcher` critical for the path of this test? And since it's wrapped in the logging check, then it wouldn't be run?
I wonder if the logic should be to have the waitForWatcher to happen, but then to wrap the logging in the if statement? Since that is the pattern that we use everywhere?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #468: ensures that test code does not depend on log level
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #468:
URL: https://github.com/apache/solr/pull/468#issuecomment-999542519
I see a mix of using `if (log.isInfoEnabled()) {` and just direct `log.info`, is there a pattern we SHOULD be using in tests?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on pull request #468: ensures that test code does not depend on log level
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #468:
URL: https://github.com/apache/solr/pull/468#issuecomment-999678226
Wether "minor" changes (this definitely is one) needs a JIRA issue is "PENDING DISCUSSION" https://cwiki.apache.org/confluence/display/SOLR/Commit+Process+Guidelines I'm a huge proponent of lowering barriers to contribute both for non-committers and us committers alike.
BTW @mariemat is my colleague and we reviewed this bug live and I approve of this fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on pull request #468: ensures that test code does not depend on log level
Posted by GitBox <gi...@apache.org>.
epugh commented on pull request #468:
URL: https://github.com/apache/solr/pull/468#issuecomment-999541524
Also, I am unclear on if a JIRA is needed. For documentation fixes we don't require a JIRA, but I think for code we do... This is maybe in a bit of a gray area!
Are there other places in the tests that have this same pattern? If there were, maybe a JIRA to cover this pattern? @janhoy ? Guidance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley merged pull request #468: ensures that test code does not depend on log level
Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #468:
URL: https://github.com/apache/solr/pull/468
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org