[Worker support] test for no cors headers #1374

madhur-tandon · 2023-04-10T14:24:34Z

for more information, see https://pre-commit.ci

pyscriptjs/tests/integration/test_00_support.py

antocuni

The code in the PR is good, apart some naming/style minor issue which I highlighted in the comments.

But the big problem is the 8000 code which is NOT in the PR: what we want is that pyscript detects the "no cors" headers and display a nice error message, with a link to the documentation which explains what is the problem.
This is just the machinery to make it possible to write a test for that, but the machinery alone is useless.

pyscriptjs/tests/integration/conftest.py

pyscriptjs/tests/integration/support.py

hoodmane · 2023-04-10T15:50:46Z

I don't think this does resolve #1312. The feature request is:

display a nice and error message (ideally with a link to a detailed page in our documentation) to explain what's the problem

When the headers are missing. As far as I can tell this patch only adjusts test code, it doesn't currently add any new error messages.

Also we should consider further improvements to synclink so that in same thread mode shared array buffer isn't needed.

hoodmane · 2023-04-10T15:53:43Z

This is a useful step towards resolving #1312 though, it gets the test support code ready.

madhur-tandon · 2023-04-10T16:12:34Z

Aah sorry, seems like I missed the part about introducing error messages in #1312

for more information, see https://pre-commit.ci

pyscriptjs/tests/integration/support.py

antocuni · 2023-04-11T08:13:50Z

@madhur-tandon I think there is a bit of confusion w.r.t the final goal. In theory, the current situation is the following:

PyScript uses Synclink which uses SharedArrayBuffer, which is available only under certain conditions
those conditions depends on the web server which servers the HTML page, and which we (PyScript OSS developers) have no control upon
if an user develops a pyscript app and deploys it to a webserver which does NOT send the required headers, the page would not work because it cannot find SharedArrayBuffer
in that case, we want to display a nice error message to the user, using an alert banner similar to what we display in case of UserError.
we want to write a test which checks that we actually display the error in case (4)

HOWEVER

I realized that we cannot write such a test currently, because situation (4) never arises: i.e., currently pyscript works perfectly fine even if we don't have CORS headers set. I think we will need it as soon as we merge PR #1333 , but not today.

@hoodmane I am very confused about what's happening on main currently. Synclink seems to have some fallback to use ArrayBuffer if SharedArrayBuffer is not available:
https://github.com/pyodide/synclink/blob/f26bfa33139bf9209cad9df5c03a4994fbc62c95/src/shared_array_buffer.ts#L1-L7

But then my question is: why did you have to modify HTTPServer in the synclink branch? In particular:

pyscript/pyscriptjs/tests/integration/conftest.py

Lines 144 to 156 in c05195c

    
           class MyHTTPRequestHandler(SimpleHTTPRequestHandler): 
        
               def end_headers(self): 
        
                   self.send_my_headers() 
        
                   SimpleHTTPRequestHandler.end_headers(self) 
        
               def send_my_headers(self): 
        
                   self.send_header("Cross-Origin-Embedder-Policy", "require-corp") 
        
                   self.send_header("Cross-Origin-Opener-Policy", "same-origin") 
        
               def log_message(self, fmt, *args): 
        
                   logger.log("http_server", fmt % args, color="blue") 
        
           host, port = "localhost", 8080

If I comment out self.send_my_header() and revert host to 127.0.0.1, the tests seems to work flawlessly. So I suppose that that change was not strictly necessary?

madhur-tandon · 2023-04-13T05:21:35Z

@antocuni Although I have added the error message, it is not in the form of a UserError / alert banner.
If you have time, can you review this again?
Although PyScript works fine even if we don't have headers set, but I made sure to still check if our dev web server / fake server supplies them or not and we then do a console.error in case they do not.

Tagging @hoodmane too for a re-review as well.

The right entry point to check for these headers is the goto() function IMO
The commit where I added the error message is here: 97bc5d5

hoodmane · 2023-04-13T06:03:42Z

So I suppose that that change was not strictly necessary?

Sounds like.

antocuni · 2023-04-13T06:46:14Z

@antocuni Although I have added the error message, it is not in the form of a UserError / alert banner.

@madhur-tandon see my previous message:
#1374 (comment)

The source of the confusion is a big misunderstanding: the synclink PR added the CORS headers to our dev server, so I assumed they were necessary for running pyscript: and in that case, the point of #1312 was to make sure that we display a nice error message to the user.

But it turns out that it's not the case: as of now, the CORS headers are not needed and pyscript works perfectly fine without them. So the "proper" fix would be to revert the change and delete the extra code from the repo, not to add a test for an unneeded feature.

However, the situation is going to change soon: CORS headers are needed for web workers, so as soon as #1333 is merged, this PR becomes relevant again. Let's put it on hold until then.

madhur-tandon · 2023-04-24T01:48:03Z

The tests that fail are kinda unrelated 😅

madhur-tandon · 2023-04-24T13:43:09Z

@antocuni and @hoodmane requesting a re-review. Thanks!

FabioRosado

Looks good to me 😄

pyscriptjs/src/main.ts

hoodmane

Looks good to me.

hoodmane · 2023-04-27T00:08:33Z

After we merge let's open an issue to add a place in the docs somewhere with more info and put a link to it in the error message.

antocuni

LGTM

pyscriptjs/tests/integration/test_01_basic.py

< 8000 svg aria-hidden="true" height="16" viewBox="0 0 16 16" version="1.1" width="16" data-view-component="true" class="octicon octicon-repo-push">

madhur-tandon and others added 2 commits April 10, 2023 19:53

test for no cors headers

5016487

[pre-commit.ci] auto fixes from pre-commit.com hooks

b645018

for more information, see https://pre-commit.ci

madhur-tandon added the sprint issue has been pulled into current sprint and is actively being worked on label Apr 10, 2023

fix tests

4ce971c

hoodmane reviewed Apr 10, 2023

View reviewed changes

pyscriptjs/tests/integration/test_00_support.py Outdated Show resolved Hide resolved

antocuni requested changes Apr 10, 2023

View reviewed changes

madhur-tandon added 2 commits April 10, 2023 23:45

suggested changes

bfd5250

disable directly

d21da0c

madhur-tandon force-pushed the cors-test branch from c31d835 to d21da0c Compare April 10, 2023 19:12

pre-commit-ci bot and others added 3 commits April 10, 2023 19:12

[pre-commit.ci] auto fixes from pre-commit.com hooks

3951f8c

for more information, see https://pre-commit.ci

add error message

97bc5d5

[pre-commit.ci] auto fixes from pre-commit.com hooks

a639654

for more information, see https://pre-commit.ci

madhur-tandon commented Apr 10, 2023

View reviewed changes

pyscriptjs/tests/integration/support.py Outdated Show resolved Hide resolved

fix conflicts

78b2356

antocuni changed the title ~~test for no cors headers~~ [Worker support] test for no cors headers Apr 13, 2023

madhur-tandon added 3 commits April 14, 2023 22:43

fix conflicts

22ce0cc

Merge branch 'main' into cors-test

bc698f2

improve test

6bb956e

FabioRosado approved these changes Apr 26, 2023

View reviewed changes

hoodmane reviewed Apr 26, 2023

View reviewed changes

pyscriptjs/src/main.ts Outdated Show resolved Hide resolved

improve error message

70ebeda

hoodmane approved these changes Apr 27, 2023

View reviewed changes

madhur-tandon requested a review from antocuni April 28, 2023 09:06

antocuni approved these changes May 2, 2023

View reviewed changes

pyscriptjs/tests/integration/test_01_basic.py Outdated Show resolved Hide resolved

remove py-config tag from cors test

40ad4fa

madhur-tandon merged commit cd1aa94 into pyscript:main May 2, 2023

madhur-tandon deleted the cors-test branch May 2, 2023 13:13

madhur-tandon self-assigned this May 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Worker support] test for no cors headers #1374

[Worker support] test for no cors headers #1374

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[Worker support] test for no cors headers #1374

[Worker support] test for no cors headers #1374

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!