Skip to content

[wasm][tests] Log messages with the correct log level#43115

Closed
radical wants to merge 2 commits into
dotnet:masterfrom
radical:wasm-logging
Closed

[wasm][tests] Log messages with the correct log level#43115
radical wants to merge 2 commits into
dotnet:masterfrom
radical:wasm-logging

Conversation

@radical

@radical radical commented Oct 6, 2020

Copy link
Copy Markdown
Member

No description provided.

.. the original messages. Instead of sending the message as is from,
say, `console.log`, we send:

`{ method: 'console.log', payload: message }`

.. which gets parsed, and logged accordingly.

Also, fix printErr to use `console.error`.
@ghost

ghost commented Oct 6, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

@radical

radical commented Oct 6, 2020

Copy link
Copy Markdown
Member Author

Corresponding XHarness PR: dotnet/xharness#328

@radical radical added the arch-wasm WebAssembly architecture label Oct 6, 2020
return function(_msg) {
window.real_print(JSON.stringify({
method: `console.${level}`,
payload: _msg

@lewing lewing Oct 6, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thoughts on passing all the args?

Suggested change
payload: _msg
payload: [...arguments]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If there is more than one argument, then how should that be logged? as the json array itself, so [ "arg1", 5, {x: 4} ]?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

preserve as much fidelity as you can?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Basically we treat console as an object so something like

realConsole[method](...payload);

@lewing lewing Oct 16, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

consider:

var message = "time";
var domain = "fun";
var fmt = "%s %s";
console.log (fmt, domain, message);
// or console.log("%s %s", "fun", "time");

the output should be

fun time

it is currently

%s %s

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

so

var payload = ["%s %s", "fun", "time"];
var method = "log";
console[method](...payload);

outputs

fun time

printErr = console.error;

// JavaScript core does not have a console defined
if (typeof console === "undefined") {

@campersau campersau Oct 7, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this check still relevant or should this be put on top?
Because now console gets also accessed if is_browser is false before this line.

If this gets changed it will conflict with #43002

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should move all the console polyfill bits to the top I think, it is getting pretty messy

@ghost

ghost commented Oct 15, 2020

Copy link
Copy Markdown

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

@lewing lewing closed this Feb 10, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants