By analyzing the blame information on this pull request, we identified @AndrewJack and @philikon to be potential reviewers.

@antoinerousseau updated the pull request.

Also, I am not sure if I should squash since there are two different authors. Let me know.

@antoinerousseau updated the pull request.

Wow, nice.
About test plan, I think you can refers to dcd922b
#6889 and #6961 created by @philikon

He provides WebSocket UIExample, so you may just add a function like sendWithCookies() as a test case

About rebase,
You can squash all merge commits into the main one along with authors.
So you would have 3 commits finally.

1. Original author's commit
2. Your coding style fixing
3. Test case function

@zxcpoiu should be all good now, tested OK :)

Great job 👍
LGTM
Now we may wait for others to review and see if it enough to pass.

remember to squash merging commit finally.

the merge commits are squashed. i guess i'll have to merge again and squash again just before the PR gets shipped?

Why I'm still seeing additional merge commit (Merge branch 'master' into android-websocket-cookie) ?

I'm not 100% sure, but as my understanding, it should be have three commits finally in this case. (I have the same confuse before 😕 )

And of course you may be asked to update your PR and rebase again.

Just follow the instructions once reviewers told you.

cc @mkonicek

@zxcpoiu I'm asking because I'm usually used to rebase my PR instead of merging master into it (for cleaner git history), but I've seen other PRs doing it, and I see an automatic Github button saying Update branch that merges the latest changes from master into this branch. ⬇️

CONTRIBUTING.md doesn't say anything about this...

I have exactly same confusion like you.

I did a little bit observe before, looking at some PR which being accepted, found out that some PR get shipped without rebased on top of the latest master commits (it's really hard to always follows the latest commits in such a highly active repo.)

That's why I am assuming that it is not necessary to rebase on top of the latest unless there are explicit reasons or conflicts need us to do so.

I guess the principle behind it are simply to keep the commit history clean, mean that each commit corresponds to one intent, one functionality or one fix rather than several commits map to one action. It makes PR easier to review by others.

Hmm, it needs someone who are familiar with that to clarify it a bit then we can help to improve instructions at Contributing.md

It doesn't matter how you merge master to your branch. The bot ultimately squashes all commits to a single one before merging.

Thank you @satya164
So

Squash your commits (git rebase -i).

is basically to provide a clear code commits for people to review, right?

And it is not necessary to keep click Merge master to your branch unless there are conflicts or be asked to do so.

is that correct?

@zxcpoiu Yes. You would want to merge master if there are conflicts or tests are failing.

these explanations should be added to CONTRIBUTING.md

I don't understand the failed CircleCI tests... any clue?

It says npm test failed, but each test failed for the same reason:

TypeError: Path must be a string. Received null
at assertPath (path.js:7:11)
at Object.dirname (path.js:1324:5)

@antoinerousseau The CirleCI failure looks unrelated. I restarted the build.

@mkonicek it keeps failing :/

Could anyone please review this? 😳 😇

why does CircleCI keeps failing? O.o

I'm not sure :{
maybe do a rebase and force push again?

It's sad that this PR got ignored 😢

Fyi I reopened a clean PR: #9114