Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #347 +/- ##
==========================================
- Coverage 93.84% 93.83% -0.01%
==========================================
Files 105 105
Lines 18218 18214 -4
==========================================
- Hits 17096 17092 -4
Misses 1122 1122
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR updates the EnvLayer.get_package_manager() unit test for Azure Linux 4 and RHEL 10 “unsupported” scenarios, aiming to address a test failure caused by stdout capture differences between PyCharm’s runner and CI.
Changes:
- Removed manual
sys.stdoutreassignment /StringIOcapture intest_get_package_manager_azure_linux_4_and_rhel10_not_supported. - Simplified assertions to only validate the empty-string return value from
get_package_manager()for unsupported distros.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for row in test_input_output_table: | ||
| self.envlayer.platform.linux_distribution = row[0] | ||
| distro.os_release_attr = row[1] | ||
|
|
||
| captured_output = io.StringIO() | ||
| sys.stdout = captured_output | ||
| result = self.envlayer.get_package_manager() | ||
| sys.stdout = sys.__stdout__ | ||
| self.assertEqual(row[2], captured_output.getvalue()) | ||
| self.assertEqual(result, "") | ||
| package_manager = self.envlayer.get_package_manager() | ||
| self.assertEqual(package_manager, "") |
kjohn-msft
added
bug
| captured_output = io.StringIO() | ||
| sys.stdout = captured_output | ||
| result = self.envlayer.get_package_manager() | ||
| sys.stdout = sys.__stdout__ |
There was a problem hiding this comment.
@yashnap, I think the problem was this line. See other examples in code where this is already implemented, for eg: test_reset_auto_assessment_log_file_if_needed_raise_exception()
you are setting sys.stdout to captured_output on line 171, but on line 174 while resetting sys.stdout it is set to "sys.__ stdout __ " (adding space because of github formatting). What does sys.__ stdout __ represent? Why not reset it to its original value?
There was a problem hiding this comment.
sys.stdout = sys._stdout - This is basically forcing stdout back to the "real" original console. captured_output = io.StringIO() was also an issue because of this : TypeError: unicode argument expected, got 'str' . One way to do it is do the way in other test you pointed out but we will be updating this code again when the actual implementation goes in and this method will return the packageManager value instead (i.e other tests in the same file) not needing the sys_out/capture_output anymore .
There was a problem hiding this comment.
@yashnap since we already have a way to test this out in code, it's always better to have a UT coverage for a code rather than not having, however small the period of code's existence be.
Also, if you are not going to validate logs, please remove log values from test_input_output_table. That is what Copilot's comment below is referring too. Removing validation on logs but still keeping it in the test matrix will just lead to confusion in the future. Always ensure your code does not leave any outliers/unreferenced sections.
rane-rajasi
left a comment
rane-rajasi
left a comment
There was a problem hiding this comment.
Follow-up on previous comment left inline
Why it Failed in Pycharm:
The test failed in PyCharm because PyCharm uses a custom unittest runner that wraps and manages sys.stdout for output capture. This conflicts with manual reassignment of sys.stdout, causing it to be replaced at runtime with a _io.TextIOWrapper that does not support getvalue(), leading to an AttributeError.
Why it passed in PR (CI) ( My assumption/Copilot Suggestion)
The test passed in the PR pipeline because it was executed using the standard Python unittest runner (python -m unittest), which does not override or wrap sys.stdout. As a result, the manual reassignment sys.stdout = StringIO() remained intact, and the captured output could be read correctly using getvalue()
The failure is due to a conflict between manual sys.stdout reassignment and PyCharm’s test runner, which overrides stdout handling, while CI uses the standard unittest runner without such interference.
-Since we will be making changes to the PackageManager code to remove this behavior and actually return a package manager value, this can be used as temporary workaround