Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
test: Add webkit test script #1627
test: Add webkit test script #1627
Changes from 9 commits
7d73f81
cfd5c4b
c3cde5e
c77c006
6cc9030
5ed2146
111ba6b
f288f82
bc177f5
36c38ba
2f0dde9
3da460d
ede58c6
3d6a67e
8e43bb5
a78db93
5a24425
c21177e
d29de4a
34cdc28
37044fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Trying to run this branch locally I noticed something weird with the stubbing but only in WebKit, though I didn't get to the bottom of it.
We stub a bunch of peer responses in each test to simulate different scenarios - once a certain number of tests were being run, the stubbed return values started being from the wrong tests.
Needs more investigation really.
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.
Yes. Hence this change. I'm not sure why exactly this fails on WebKit but seems to work on other browsers. I'm not sure overriding values is supported in Sinon or if it's a hack we do. The change here is to not override and instead be explicit in what gets stubbed vs set.
My guess is the other failures we're seeing is related to something like this.
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.
Have you checked out the Sinon docs? I believe we’re using it as intended, no “hack” involved.
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 think sinon is not involved here. This should just be setting values on the object here: https://github.com/ttarnowski/ts-sinon/blob/master/src/index.ts#L67. Maybe
Proxy
in WebKit has slightly different behavior? Maybe a bug since this only shows up in certain cases (e.g. run only this test is fine, but run two versions of this test fails).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, it’s really weird because we don’t reuse the stubbed instances between the tests that I can see 🤔
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.
@maschad did you figure out what the issue was here around Proxy?
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.
@MarcoPolo I don't think the issue was related to
Proxy
but rather the peer id fixtures, the relay server behaved as if it was creating reservations for peers that seemed to be reconnecting due to the tests reusing peer ids, we resolved this in this PR