-
Notifications
You must be signed in to change notification settings - Fork 766
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 g:ycm_roslyn_binary_path being ignored #1721
Conversation
d7eca35
to
6ef84d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally, this looks good.
I did leave one comment.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @dkaszews)
ycmd/tests/cs/debug_info_test.py
line 207 at r1 (raw file):
def _RoslynFromUserOption_popen_mock(*args, **kwargs):
This will fail flake8 check.
( *args, **kwargs )
- mind the "floating" parentheses.
Also, I'm too hungry to take a closer look now, but...
if this helper function needs to be a member of the test class, I would prefer to see self, *args, **kwargs
.
If it needs not be a member of the test class, let's move it above the test class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
ycmd/tests/cs/debug_info_test.py
line 207 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
This will fail flake8 check.
( *args, **kwargs )
- mind the "floating" parentheses.Also, I'm too hungry to take a closer look now, but...
if this helper function needs to be a member of the test class, I would prefer to see
self, *args, **kwargs
.
If it needs not be a member of the test class, let's move it above the test class.
It doesn't need to be member of test class, but I would like to keep it next to the test case as it has same hardcoded value, basically a multiline lambda. Maybe @staticmethod
then?
I also just had a stupid idea to check out tommorow. We only care about value here and the forward to real Popen
is just to keep rest of test of failing. Just throw some custom exception that will only be caught in test and end it early. Should save a lot of time by not actually starting the server and waiting on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if CI has come to its senses.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @dkaszews)
ycmd/tests/cs/debug_info_test.py
line 207 at r1 (raw file):
Previously, dkaszews (Dominik Kaszewski) wrote…
It doesn't need to be member of test class, but I would like to keep it next to the test case as it has same hardcoded value, basically a multiline lambda. Maybe
@staticmethod
then?I also just had a stupid idea to check out tommorow. We only care about value here and the forward to real
Popen
is just to keep rest of test of failing. Just throw some custom exception that will only be caught in test and end it early. Should save a lot of time by not actually starting the server and waiting on it.
@staticmethod
sounds good to me.
Also, we have merged the unicode/regex update, so you should rebase. |
6ef84d3
to
67274ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed and rebased
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)
ycmd/tests/cs/debug_info_test.py
line 207 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
@staticmethod
sounds good to me.
Done, the exception idea unfortunatelly did not work, as it was caught by production code I don't have time to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @dkaszews)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)
Apparently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just drop @staticmethod
and add a self
as the first argument, which it would have anyway. Then just add a comment about it too.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)
That did not work either, was getting errors that instance method was called without |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)
ycmd/tests/cs/debug_info_test.py
line 212 at r5 (raw file):
# `@patch` does not play nice with functions defined at class scope def _popen_mock( *args, **kwargs ): assert_that( args[ 0 ][ 1 ], equal_to( 'my_roslyn.exe' ) )
given that we're using args[0]
directly, why not:
def _popen_mock( cmdline, *args, **kwargs ):
# use cmdline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied suggestion
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)
ycmd/tests/cs/debug_info_test.py
line 212 at r5 (raw file):
Previously, puremourning (Ben Jackson) wrote…
given that we're using
args[0]
directly, why not:def _popen_mock( cmdline, *args, **kwargs ): # use cmdline
Fair point, done. I omitted *args
because all other arguments are named-only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)
Any clue why this fails only on older Windows? |
I have re-run them. Let's see if it was CI flake. |
Nope, looks like the test really does just fail on windows:
|
Could not get through the callstack myself, but now it just looks like it fails because on Windows it runs with |
yeah, it's a bit cryptic the way the failure is reported! |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1721 +/- ##
=======================================
Coverage 95.45% 95.45%
=======================================
Files 83 83
Lines 8136 8143 +7
Branches 163 163
=======================================
+ Hits 7766 7773 +7
Misses 320 320
Partials 50 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @dkaszews)
ycmd/tests/cs/debug_info_test.py
line 212 at r7 (raw file):
# `@patch` does not play nice with functions defined at class scope def _popen_mock( cmdline, **kwargs ): i = cmdline[ 0 ].endswith( 'mono' )
not a fan of this. I would rather:
exe_idx = 1 if cmdline[ 0 ].endswith( 'mono' ) else 0
Because that says what it is doing, rather than assuming you implicitly know that, and that the True will turn into 1
when used as an index... which is weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 2 LGTMs obtained (and 2 stale) (waiting on @puremourning)
ycmd/tests/cs/debug_info_test.py
line 212 at r7 (raw file):
Previously, puremourning (Ben Jackson) wrote…
not a fan of this. I would rather:
exe_idx = 1 if cmdline[ 0 ].endswith( 'mono' ) else 0
Because that says what it is doing, rather than assuming you implicitly know that, and that the True will turn into
1
when used as an index... which is weird.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's squash.
Reviewed 1 of 1 files at r9.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)
-- commits
line 28 at r9:
let's squash the commits now, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)
@puremourning Do you need me to squash manually? I'm AFK so might be a bit hard. The codecov fails seem to be because I rewrote bits of code to be shorter so the line percentage went down 🙄 |
@Mergifyio squash |
Fix g:ycm_roslyn_binary_path being ignored * Add test * Clean test * Fix formatting * Fix UT * Fix UT for Python 3.8 * Make popen mock args more readable * Fix UT on Windows * Explicit index * Explicit index name
✅ Pull request squashed successfully |
a6e4146
to
e318b5e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dkaszews Thanks for the pull request!
Reviewed 1 of 1 files at r4.
Reviewable status: 1 of 2 LGTMs obtained (and 1 stale) (waiting on @dkaszews)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @dkaszews)
Thanks for sending a PR! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: complete! 2 of 2 LGTMs obtained (waiting on @dkaszews)
Thanks for sending a PR! |
Fixes the
g:ycm_roslyn_binary_path
only being used in prechecks, and the actual server always being started from theycmd/third_party
directory.Also fixes the
:YcmDebugInfo
printing the directory containing default OmniSharp executable (and not the executable file itself); now prints full command with arguments used to start the server.This change is