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.

Sign up for GitHub

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

Jump to bottom

Removes dead code. #300

Merged

homu merged 1 commit into ycm-core:master from oblitum:dead-code

Jan 11, 2016

Contributor

oblitum commented Jan 9, 2016

Since regex triggers have been enabled, _StringTriggerMatches was
actually not in use anymore since _PrepareTrigger always return
regexes, making triggers never a instance of basestring.

Contributor Author

oblitum commented Jan 9, 2016

This also makes the old tests to follow the code path that actually happens in "production".

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io

Contributor

vheon commented Jan 9, 2016

I'm just worried that we're testing two methods at the same time 😕

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io

Contributor Author

oblitum commented Jan 9, 2016

I'm a bit unsure how it should be split, notice that MatchesSemanticTrigger_RegexTrigger_test also seems like an example of this, both string and regex matching requires the prepare step.

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io

Contributor

vheon commented Jan 9, 2016

Is true, and is also true that the _PrepareTrigger is really simple

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io

Contributor

homu commented Jan 9, 2016

☔ The latest upstream changes (presumably #292) made this pull request unmergeable. Please resolve the merge conflicts.

Contributor Author

oblitum commented Jan 10, 2016

Should I worry about this coverage thing?

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io

Contributor Author

oblitum commented Jan 10, 2016

I mean, in this case.

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io

Member

Valloric commented Jan 10, 2016

@oblitum In general, deleting code that had test coverage that was higher than the project average will always bring down the project average coverage. So don't worry about it.

Contributor Author

oblitum commented Jan 10, 2016

@Valloric ok, thanks.

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io

Contributor

vheon commented Jan 10, 2016

@oblitum have you rebased on top of the other PR you made? I think that this otherwise

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io

Contributor

vheon commented Jan 10, 2016

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.

Comments from the review on Reviewable.io


          Removes dead code.

9bceb9d

Since regex triggers have been enabled, _StringTriggerMatches was
actually not in use anymore since _PrepareTrigger always return
regexes, making triggers never a instance of basestring.

Contributor Author

oblitum commented Jan 10, 2016

@vheon did it again now.

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from the review on Reviewable.io

Member

puremourning commented Jan 11, 2016

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from the review on Reviewable.io

Member

Valloric commented Jan 11, 2016

Hm, we seem to be getting some flakiness from typescript tests...

Contributor

homu commented Jan 11, 2016

📌 Commit 9bceb9d has been approved by Valloric

Contributor

homu commented Jan 11, 2016

⌛ Testing commit 9bceb9d with merge b195c0f...

homu added a commit that referenced this pull request


          Auto merge of #300 - oblitum:dead-code, r=Valloric

b195c0f

Removes dead code.

Since regex triggers have been enabled, _StringTriggerMatches was
actually not in use anymore since _PrepareTrigger always return
regexes, making triggers never a instance of basestring.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/300)
<!-- Reviewable:end -->

Contributor

homu commented Jan 11, 2016

☀️ Test successful - status

homu merged commit 9bceb9d into ycm-core:master

oblitum deleted the dead-code branch

January 11, 2016 06:11

Contributor

vheon commented Jan 11, 2016

Hm, we seem to be getting some flakiness from typescript tests...

I think we hit a threading bug in the completer even though we have a lock there 😕

Member

puremourning commented Jan 11, 2016

I looked at this before, as I hit it too. It looks like we basically take concurrency down to 1 with the self._lock. I wonder if something has changed in tsserver, such as sending additional (unsolicited) messages that we aren't expecting.

We don't lock the version of typescript in travis_install.sh (or recommend doing so in our readme), so we're at the mercy of the version of typescript that gets installed.

npm install -g typescript

We could at least improve the diagnostics if it keeps happening.

Member

Valloric commented Jan 12, 2016

Flaky tests are a huge pet peeve of mine. I strongly believe such tests add negative value so I tend to either fix the flakiness behind them or delete the tests involved. So if this continues to happen and nobody tackles it before I get to it, I'll take a look at it and one of those two will happen. :)

We can't have flaky tests. They make the whole test suite pointless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet