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

Fix test_compare_to_baseline_equal #2296

Closed
wants to merge 1 commit into from

Conversation

mpolson64
Copy link
Contributor

Summary:
Remove log checking from test_compare_to_baseline_equal

Context: D54885250 moved some logs from INFO to DEBUG, breaking this test which checked for logs at the INFO level. However, we cant just change the level in the assertLogs context manager to DEBUG because DEBUG logs are silent by default in Ax -- the default log level is INFO.

IMO we should just remove this test, and maybe remove the logs too. Right now its impossible for them to be seen by the user, and will only be visible to developers if they manually change the level of the logger in report_utils in source. If we were concerned enough about logspew to move these out of INFO, maybe we should just get rid of them altogether. Thoughts?

Reviewed By: bernardbeckerman

Differential Revision: D55248686

Summary:
Remove log checking from test_compare_to_baseline_equal

Context: D54885250 moved some logs from INFO to DEBUG, breaking this test which checked for logs at the INFO level. However, we cant just change the level in  the assertLogs context manager to DEBUG because *DEBUG logs are silent by default in Ax* -- the default log level is INFO.

IMO we should just remove this test, and maybe remove the logs too. Right now its impossible for them to be seen by the user, and will only be visible to developers if they manually change the level of the logger in report_utils in source. If we were concerned enough about logspew to move these out of INFO, maybe we should just get rid of them altogether. Thoughts?

Reviewed By: bernardbeckerman

Differential Revision: D55248686
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Mar 22, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D55248686

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 3a701af.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants