-
-
Notifications
You must be signed in to change notification settings - Fork 43
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
Notify proposal state changes in general chatroom #2147
Notify proposal state changes in general chatroom #2147
Conversation
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.
Nice work @Silver-IT ! Preliminary review.. see comments
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.
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.
Leaving this comment here so that it can be referenced from another issue
@@ -498,7 +498,7 @@ describe('Group Chat Basic Features (Create & Join & Leave & Close)', () => { | |||
cy.giLogout() | |||
}) | |||
|
|||
it(`user2 joins the ${groupName1} group and ${CHATROOM_GENERAL_NAME} again and logout`, () => { | |||
it(`user2 joins the ${groupName1} group and ${CHATROOM_GENERAL_NAME} channel again and logout`, () => { |
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.
On my local test run the last test in group-chat.spec.js
(user2 joins the ${groupName1} group and ${CHATROOM_GENERAL_NAME} channel again and logout
) failed:
group-chat.spec.js.mp4
This is failing here inside of cy.giAcceptGroupInvite
:
if (existingMemberUsername) {
// NOTE: checking 'data-groupId' is for waiting until joining process would be finished
cy.getByDT('pendingApprovalTitle').invoke('attr', 'data-groupId').should('eq', groupId)
// NOTE: should wait until KEY_REQUEST event is published
cy.giKeyRequestedGroupIDs(groupId)
cy.giEmptyInvocationQueue() // <---------remove
cy.giLogout()
And then in:
Cypress.Commands.add('giLogout', ({ hasNoGroup = false } = {}) => {
cy.giEmptyInvocationQueue()
if (hasNoGroup) {
cy.window().its('sbp').then(async sbp => await sbp('gi.app/identity/logout'))
} else {
cy.getByDT('settingsBtn').click() // <------ failing here
There are multiple problems here:
cy.giEmptyInvocationQueue()
is being called an extra time beforecy.giLogout()
(which also calls it) - so that unnecessary call ingiAcceptGroupInvite
on linecommands.js:498
should be removed.- And the main problem: for some reason we were given the key to join the group and so the approval screen is visible here and the settings button to log out cannot be clicked, and also there is no need for it be clicked because we're already in the group
It might be because user2
was already a member of the group before, and so was somehow able join again because the keys were not properly rotated when he left in 'user2 leaves the group by himself'
.
However, running the tests a second time everything passed, so this is a challenging and important Heisenbug that needs to be fixed. I will open an issue for this.
cc @corrideat
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.
Yeah, that makes sense. I remember I have already encountered the second issue several times before. Also it doesn't only happen in group-chat.spec.js
, and I remember it was happening in any other test cases too.
Since you created an issue for this heisenbug, we can check it out and work on it later.
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.
I'd like to leave this unresolved just so that when the issue links to it this conversation can be seen.
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.
Request for comment plus one more bug fix request
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.
A few minor change requests, but also please see this new Cypress failure that this PR is creating: https://cloud.cypress.io/projects/q6whky/runs/2699/test-results/85bbdf50-dca9-4e35-ab5d-ede8d5f4ab3e/video?actions=%5B%5D&browsers=%5B%5D&groups=%5B%5D&isFlaky=%5B%5D&modificationDateRange=%7B%22startDate%22%3A%221970-01-01%22%2C%22endDate%22%3A%222038-01-19%22%7D&orderBy=EXECUTION_ORDER&oses=%5B%5D&specs=%5B%5D&statuses=%5B%7B%22value%22%3A%22FAILED%22%2C%22label%22%3A%22FAILED%22%7D%5D&testingTypesEnum=%5B%5D&utm_source=github
It looks like a heisenbug that depends on the order of messages in the chat perhaps. Or on the scroll position of the chatroom maybe?
Either way, it needs to be fixed because it seems quite frequent.
I have already added a comment in
|
It depends on the order of messages, not the scroll position. |
Can I create an issue for this and work on it later?
|
I agree we need to merge this PR asap, but we also need the tests to be regularly passing and this PR introduces serious and regular Heisenbugs. We already have heisenbugs. Please give it at least one more day of trying. You can try to change the tests so that they are not written in such a way that they depend on the order of messages so much. |
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.
Nice work @Silver-IT!
Summary of Changes