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

[wasm] write test as single line via base64 #771

Merged
merged 4 commits into from
Nov 22, 2021

Conversation

pavelsavara
Copy link
Member

No description provided.

@radical
Copy link
Member

radical commented Nov 19, 2021

What kind of errors is this guarding against? Invalid xml only? We should catch those specific exceptions then.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, forgot to submit!

@pavelsavara pavelsavara changed the title [wasm] write test results xml file even if it's invalid [wasm] write test as single line via base64 Nov 20, 2021
@pavelsavara
Copy link
Member Author

pavelsavara commented Nov 20, 2021

After the last observation I realized that

  • we still have multiple lines of console.log and therefore WS send with STARTRESULTXML and ENDRESULTXML on separate lines.
  • we still have multiple lines because xml file could contain <![CDATA[some \n stuff]]>

Therefore I made it really one line, which also simplified dealing with race condition (with lines from Chrome DevTools Protocol) on receiving side.

Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you able to reproduce the issue locally? And does this fix it?

We should probably come up with a better way of doing this. Can we read files from the VFS after the app exits, but the page is still open?

@radical
Copy link
Member

radical commented Nov 21, 2021

The results file for System.Memory.Tests is ~12MB, which becomes ~16M in base64. If @lewing's dotnet/runtime#61854 works, with a chrome update from dotnet/runtime#61856 , then we should use that, and write lines as we did earlier.

@radical radical requested a review from premun November 21, 2021 04:52
@pavelsavara
Copy link
Member Author

The results file for System.Memory.Tests is ~12MB, which becomes ~16M in base64. If @lewing's dotnet/runtime#61854 works, with a chrome update from dotnet/runtime#61856 , then we should use that, and write lines as we did earlier.

I commented on 61854, I doubt it would help.

I think that 16MB should be fine. The ugly part is that emscripten stdout will do put_char on all of that, but we already have this problem and I only made it slightly worse.

If we get back to writing multiple lines, we need to prevent the race condition with lines from "chrome devtools protocol" by other means. My previous adding isWebSocket is one possible way.

@pavelsavara
Copy link
Member Author

pavelsavara commented Nov 21, 2021

Were you able to reproduce the issue locally?

No, not yet. Were you ?

We should probably come up with a better way of doing this. Can we read files from the VFS after the app exits, but the page is still open?

The shell JS like V8 doesn't have this issue as far as I could tell.

For the browser, we could replace this with HTTP POST to xharness web server and save it there.
It would avoid all the contention and also all the buffering of stdout.
I'm not sure what would it take to bring HttpClient as new dependency of ThreadlessXunitTestRunner

@maraf
Copy link
Member

maraf commented Nov 22, 2021

For the browser, we could replace this with HTTP POST to xharness web server and save it there.
It would avoid all the contention and also all the buffering of stdout.
I'm not sure what would it take to bring HttpClient as new dependency of ThreadlessXunitTestRunner

I really like this HTTP idea.

@radical
Copy link
Member

radical commented Nov 22, 2021

For the browser, we could replace this with HTTP POST to xharness web server and save it there.
It would avoid all the contention and also all the buffering of stdout.
I'm not sure what would it take to bring HttpClient as new dependency of ThreadlessXunitTestRunner

I really like this HTTP idea.

we could even write it back over a different web socket, if we want to avoid using HttpClient.

@pavelsavara
Copy link
Member Author

pavelsavara commented Nov 22, 2021

The problem is in how to get out of wasm into JS. In JS the http is the easy part.
Currently we are abusing stdout.
Instead C# httpClient we could also recognize the pattern in JS at same place we have WS proxy and POST it from there.

I would merge this one so that we could iterrate with testing in in CI and will create another PR exploring this idea.

@pavelsavara
Copy link
Member Author

we could even write it back over a different web socket, if we want to avoid using HttpClient.

I would like to avoid using C# HttpClient because I don't want to force the DLL dependency on all users of ThreadlessXunitTestRunner. On the other hand abusing stdout is no great either.

@pavelsavara pavelsavara merged commit ce11017 into dotnet:main Nov 22, 2021
@pavelsavara
Copy link
Member Author

I realized that we could combine multiple ideas.
@radical mentioned VFS.
We could use it to get data into JS and then we collect it via JS and upload with HTTP POST.

@radical
Copy link
Member

radical commented Nov 22, 2021

We could call a js function which would return the contents of the file from the VFS, or we could write that to some dom element, and read it from XHarness. We connect through selenium, so we can "talk" to the page.

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.

3 participants