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

fix(messaging): fix inject error #551

Merged
merged 7 commits into from
Nov 9, 2022
Merged

fix(messaging): fix inject error #551

merged 7 commits into from
Nov 9, 2022

Conversation

allardy
Copy link
Member

@allardy allardy commented Nov 3, 2022

Pretty sure the error is related to the "isWebchatEvent" function... It was really too basic.
image

These event causes those errors:
image

Still need a way to test them correctly, since stratus uses the CDN, I didn't test "strictly", but logic looks fine.

@allardy
Copy link
Member Author

allardy commented Nov 3, 2022

@spg
Copy link
Contributor

spg commented Nov 3, 2022

Adding tests for that function would be pretty easy. Should we do this now?

@allardy
Copy link
Member Author

allardy commented Nov 3, 2022

Agreed that tests would be nice, but i'm not really used to DOM testing. Could be a good challenge for someone who implements new features in the future

@spg
Copy link
Contributor

spg commented Nov 3, 2022

I was not thinking of DOM testing, but only testing some functions of inject.ts file. Here, the isWebchatEvent() could benefit from a few simple tests

@allardy
Copy link
Member Author

allardy commented Nov 9, 2022

@spg Tests added.

I disabled the firefox test because it's broken and has been for a long time, 6h running time is abusive. Fixing the real error should be done in a sepatate pr

image
image

@spg
Copy link
Contributor

spg commented Nov 9, 2022

@allardy please add a ticket in Linear so we remember to fix the tests

@allardy allardy merged commit 87a8ef5 into master Nov 9, 2022
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