Skip to content

put all websocket sessions in a single static map#306

Merged
line-o merged 7 commits intoeXist-db:masterfrom
nverwer:#305-Console_can_only_handle_one_session_at_a_time
Aug 18, 2025
Merged

put all websocket sessions in a single static map#306
line-o merged 7 commits intoeXist-db:masterfrom
nverwer:#305-Console_can_only_handle_one_session_at_a_time

Conversation

@nverwer
Copy link
Copy Markdown
Contributor

@nverwer nverwer commented Jun 30, 2025

This fixes #305: The console:log function could only send to one channel, because it could only reach one session. By putting all sessions in a static map, console:log can find the correct session belonging to the channel it wants to log to.

Copy link
Copy Markdown
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nverwer thank you for your contribution, much appreciated.

My only concern is that the desired behaviour isn't tested with the PR, so it will be tricky to maintain going forward. If you need help with creating a test within the project set-up let us know and we can figure out where to best to include it.

I've added the PR to the agenda of next weeks community call.

@nverwer
Copy link
Copy Markdown
Contributor Author

nverwer commented Jul 6, 2025

Thank you, @duncdrum . I am working on different projects, and it might take some time for me to figure out how to make an effective test. I will have a look as soon as I find the time for it.

Copy link
Copy Markdown
Member

@line-o line-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this seven character fix is blocked by a missing test. I would still want to get a test for it but this should not prevent this from going in.

@dizzzz dizzzz requested a review from duncdrum July 14, 2025 18:36
@duncdrum
Copy link
Copy Markdown
Contributor

@nverwer we discussed this PR in the community call if you can't find the time for adding the test. I'll add one via cypress once I can find the time.

@duncdrum duncdrum self-assigned this Jul 15, 2025
@adamretter
Copy link
Copy Markdown
Contributor

adamretter commented Jul 15, 2025

@duncdrum I remember the conclusion at the Community Call differently. I recall that we discussed that this could be tested manually, as we don't have the necessary test infrastructure (that @nverwer would need) available in Java within the project; that we could be pragmatic, and merge it if there is no issue identified from manual testing. I also on the call confirmed that I had previously tested this manually already and seen no issue.

@duncdrum
Copy link
Copy Markdown
Contributor

@adamretter It is correct that you asserted that this couldn't be tested on the existing infrastructure, we took a look and disagreed. This can be tested from within the existing test infrastructure, and so it should be. You are of course free to expand the

necessary test infrastructure available in Java

in a separate PR.

@adamretter
Copy link
Copy Markdown
Contributor

we took a look and disagreed

There is no infrastructure in Monex for either Unit or Integration tests written in Java at present. @duncdrum How do you see this working?

@duncdrum
Copy link
Copy Markdown
Contributor

@adamretter you don't have to test this from within Java. We have integration test configured so I m planning to expand those.

@nverwer
Copy link
Copy Markdown
Contributor Author

nverwer commented Jul 16, 2025 via email

@nverwer
Copy link
Copy Markdown
Contributor Author

nverwer commented Jul 24, 2025

I will be back next week, and see if I can come up with a unit test.

When I wrote that, I clearly had no idea what writing a test would involve, because I have never used Cypress.
With the help of an AI, I made a test, but running it (with mvn release:prepare) results in errors that I am unable to fix.

I pushed my test, so that someone who knows more than I do about this stuff may try to run it.

@duncdrum
Copy link
Copy Markdown
Contributor

@nverwer thank you. I ll refactor this, I was out and about.

@duncdrum duncdrum force-pushed the #305-Console_can_only_handle_one_session_at_a_time branch from bc96b81 to 9c64469 Compare July 28, 2025 10:12
Copy link
Copy Markdown
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now with tests and green

@duncdrum
Copy link
Copy Markdown
Contributor

@nverwer the changes to the test code are all in here 9c64469 I took the chance to upgrade the framework, and had to rebase.

@eXist-db/core we should cut a release after this is merged, the original change fixes another failing test with console:log

Copy link
Copy Markdown
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test looks good. However the default port should not be 8080. Also that is already solved in this earlier PR #298 which will need to be merged before this one, and then this one will need to be rebased.

Comment thread cypress.config.js
@duncdrum duncdrum dismissed adamretter’s stale review July 28, 2025 11:50

mixing concerns

Copy link
Copy Markdown
Contributor

@adamretter adamretter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to have the port issue fixed as it hardcodes it in many places.

@line-o line-o mentioned this pull request Jul 31, 2025
@line-o
Copy link
Copy Markdown
Member

line-o commented Aug 17, 2025

I will pull this in as #298 is blocked

@adamretter
Copy link
Copy Markdown
Contributor

@line-o this PR should not get pulled in as is. It needs to be broken into 2 PRs. This one should only have the Websocket fix and test, it should not have the Cypress upgrade and other test refactorings in it

@line-o
Copy link
Copy Markdown
Member

line-o commented Aug 18, 2025

I would pull in both PRs so I don't see the point of splitting them first.

@line-o line-o merged commit eb26bca into eXist-db:master Aug 18, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Console can only handle one session at a time

5 participants