comlink icon indicating copy to clipboard operation
comlink copied to clipboard

Improve two-way communication test

Open Nemikolh opened this issue 2 years ago • 0 comments

Hello! :wave:

TLDR;

This PR improves the two-way communication test to fail if WireValueType and MessageType would happen to overlap again in the future.

Full story

First, I want to thank you for your work on Comlink. This is one of the most elegant, well written library I've ever come across :star: :clap:

I recently noticed that the test for two-way communication does not do enough work to actually fail in versions of Comlink < 4.3.1. I encountered this issue while trying to port two-way communication to our fork which is stuck at 4.3.0.

To see how this PR helps you can use this bash script:
# To run this, you need jq, npm, git

# Checkout this branch
BRANCH=fix-two-way-iframe-test
git remote add Nemikolh [email protected]:Nemikolh/comlink.git
git fetch Nemikolh
git checkout $BRANCH || { echo "Oopsie"; exit 1; }

# Hacky way to keep our test around
mkdir -p tmp/fixtures
cp tests/tsconfig.json                  tmp/
cp tests/type-checks.ts                 tmp/
cp tests/two-way-iframe.comlink.test.js tmp/
cp tests/fixtures/two-way-iframe.html   tmp/fixtures/
cp package.json package.json.tmp
cp tsconfig.json tsconfig.json.tmp
cp rollup.config.mjs rollup.config.mjs.tmp

# Checkout v4.3.0
git checkout v4.3.0 || { echo "Oopsie"; exit 1; }

# Make sure we run our latest tests
rm -rf tests && mv tmp tests
mv package.json.tmp      package.json
mv rollup.config.mjs.tmp rollup.config.mjs && rm rollup.config.js
mv tsconfig.json.tmp     tsconfig.json

# Use older TypeScript version (otherwise build fails)
echo "$(jq '.devDependencies.typescript = "4.6.4"' package.json)" > package.json

# Run those tests on v4.3.0
npm install
npm test

# Cleanup
rm rollup.config.mjs
git clean -f tests
git restore tests
git restore package.json
git restore tsconfig.json
git restore rollup.config.js

# Move back to the branch and run the tests again
git checkout $BRANCH
npm install
npm test

If you run this script you'll see this error in your console (you might need to scroll up a bit :scroll:):

AssertionError: expected DOMException{ 
     stack: 'Error: Failed to execute \'postMessage\' on \'Window\': 
                 A MessagePort could not be cloned because it was not transferred.\n   
      at Object.postMessage (dist/esm/comlink.mjs:240:48)\n    
      at dist/esm/comlink.mjs:126:16' 
} to equal ''
at Context.<anonymous> (tests/two-way-iframe.comlink.test.js:61:22)

If you run the script again but replace BRANCH=fix-two-way-iframe-test by origin/main (or equivalent), you can see that the test pass (you'll need to scroll up again to view the result of the first karma start):

23 02 2023 19:46:55.221:INFO [launcher]: Starting browser ChromeHeadless
23 02 2023 19:46:55.479:INFO [Chrome Headless 110.0.5481.100 (Linux x86_64)]: Connected on socket MGZ7B5cMMyiLbvGDAAAB with id 54251662
Chrome Headless 110.0.5481.100 (Linux x86_64): Executed 1 of 1 SUCCESS (0.019 secs / 0.005 secs)

This shows that the test is missing a few bits.

Nemikolh avatar Feb 23 '23 19:02 Nemikolh