Skip to content

lsp-test: add setIgnoringProgressNotifications#566

Draft
jhrcek wants to merge 1 commit into
haskell:masterfrom
jhrcek:jhrcek/ignore-progress-notification-dynamically
Draft

lsp-test: add setIgnoringProgressNotifications#566
jhrcek wants to merge 1 commit into
haskell:masterfrom
jhrcek:jhrcek/ignore-progress-notification-dynamically

Conversation

@jhrcek

@jhrcek jhrcek commented Apr 7, 2024

Copy link
Copy Markdown
Collaborator

@michaelpj

Copy link
Copy Markdown
Collaborator

Did this achieve what you wanted in the tests?

@jhrcek

jhrcek commented Apr 9, 2024

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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

jhrcek commented Apr 10, 2024

Copy link
Copy Markdown
Collaborator Author

@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
Copy Markdown
Collaborator

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

@jhrcek

jhrcek commented Apr 16, 2024

Copy link
Copy Markdown
Collaborator Author

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.

2 participants