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

lsp-test: add setIgnoringProgressNotifications #566

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhrcek
Copy link
Collaborator

@jhrcek jhrcek commented Apr 7, 2024

@michaelpj
Copy link
Collaborator

Did this achieve what you wanted in the tests?

@jhrcek
Copy link
Collaborator Author

jhrcek commented Apr 9, 2024

Not sure yet.
I have a PR in hls repo with upgrade of hls to master version of lsp: haskell/haskell-language-server#4166
I have a PR in my fork which uses this PR branch: jhrcek/haskell-language-server#8

Both have test failures, but I didn't have time yet to look into those. Will check them later this week.

I'll probably just revert to your idea of rebasing the lsp PR on some commit before the metamodel update to make it easier for myself.

@michaelpj
Copy link
Collaborator

It would be nice to know since I'll probably release the packages soon so that HLS can adapt to the changes you helpfully fixed :)

@jhrcek
Copy link
Collaborator Author

jhrcek commented Apr 10, 2024

@michaelpj Give me one more day. I'll try to fix up test failures in jhrcek/haskell-language-server#9, which uses lsp patch applied before metamodel update to minimize the amount of breakage that other recent lsp changes seems to have introduced.

@michaelpj
Copy link
Collaborator

To be clear, when I say "soon" I don't mean that soon. Maybe next week or something.

@jhrcek
Copy link
Collaborator Author

jhrcek commented Apr 16, 2024

Status update: we decided not to merge it yet, because it would require enabling progress notifications in many hls tests.

Here's a test PR that makes the hls tests pass by enabling listening to progress notifications within "waitForProgress" helper functions.
But this strategy feels too brittle (e.g. it probably allows for races where notification is sent sooner than waitForProgress is called). Probably more robust approach would require enabling progress notifications in session config of all tests that actually need to wait for progress to be completed OR refactoring some of those tests not to rely on progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants