-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fix Cypress failures #2063
Fix Cypress failures #2063
Conversation
Passing run #2396 ↗︎
Details:
Review all test suite changes for PR #2063 ↗︎ |
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.
Ready for the review!
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.
Thanks @Silver-IT! I left a question
if (this.isInCypress) { | ||
// NOTE: When the user's actions are very quick, that can logout before to save `readUntilMessageHash`, | ||
// we should save `readUntilMessageHash` before to update contract state. | ||
// This normally happens in Cypress, when user logs out just after sending a message. | ||
const curMessages = newContractState.messages || [] | ||
if (addedOrDeleted === 'ADDED' && curMessages.length) { | ||
const lastMessage = curMessages[curMessages.length - 1] | ||
this.updateReadUntilMessageHash({ | ||
messageHash: lastMessage.hash, | ||
createdHeight: lastMessage.height | ||
}) | ||
} | ||
} |
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.
Even with the comment, it's not clear to me why this code is here.
- What issue is this fixing, and which Cypress test would be failing without it?
- Is it possible to fix this in some other way, perhaps by modifying the Cypress tests themselves and having them wait on something?
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.
What issue is this fixing, and which Cypress test would be failing without it?
This was a very picky situation which kept making Cypress to be failed. Let me explain.
Once we receive the message event(e.g., 'gi.contracts/chatroom/addMessage'
), we process the event in this function and the this.messageState
is updated. So the computed value this.messages
would be updated.
After that, we save the readUntil
message to the KV store by calling this.updateReadUntilMessageHash
function.
Now, let's think about Cypress test scenario to logout as soon as to send a message. So the workflow is like the following.
- User sends a message
- Cypress waits for the message to be added (
pending
now) - Cypress waits for the message status to be
sent
(This is when the event is received, processed andthis.messageState
is updated) - (Cypress decides the
Sending a message
is finished, actually is.) Call cy.giLogout()
So the Cypress didn't wait for the this.updateReadUntilMessageHash
to be called, and also for the invocations from the queue UNREAD_MESSAGES_QUEUE
. The invocations from UNREAD_MESSAGES_QUEUE
run the following sbp function, so it would break Cypress whenever the invocations were called after logout.
return sbp('chelonia/kv/set', ourIdentityContractId, KV_KEYS.UNREAD_MESSAGES, data, {
encryptionKeyId: sbp('chelonia/contract/currentKeyIdByName', ourIdentityContractId, 'cek'),
signingKeyId: sbp('chelonia/contract/currentKeyIdByName', ourIdentityContractId, 'csk'),
})
Adding await sbp('okTurtles.eventQueue/queueEvent', UNREAD_MESSAGES_QUEUE, () => {})
in the gi.actions/identity/logout
function, couldn't solve the problem because a new invocation could be added after we call the gi.actions/identity/logout
function. When the user's actions are very quick, only in Cypress. Look at the following orders of function calls
- Update this.messageState in ChatMain.vue
- Call
gi.actions/identity/logout
function - Call
await sbp('okTurtles.eventQueue/queueEvent', UNREAD_MESSAGES_QUEUE, () => {})
- Call
this.updateReadUntilMessageHash
and add new invocation toUNREAD_MESSAGES_QUEUE
Is it possible to fix this in some other way, perhaps by modifying the Cypress tests themselves and having them wait on something?
I did also tried to find another way to wait for this in Cypress, but couldn't find alternative solution which doesn't use cy.wait()
.
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 have just had an inspiration that add exception handler in fetchChatRoomUnreadMessages
and saveChatRoomUnreadMessages
function.
EDITED: I just realised that the exception handler couldn't solve the problem too. Because it's necessary to wait for the UNREAD_MESSAGES_QUEUE
invocations to be finished before user logs out, and those kinds of exceptions (which would be related to the identityContractID) should not be thrown. So far, I couldn't find the alternative solution.
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.
@taoeffect, I will re-write the test scenario and remove the picky cases.
Summary of Changes