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.

Sign up for GitHub

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

Jump to bottom

[wasm] Wasm.Build.Tests - fixes for tests failing on CI #70704

Merged
merged 7 commits into from
Jun 30, 2022
Merged

[wasm] Wasm.Build.Tests - fixes for tests failing on CI #70704

merged 7 commits into from
Jun 30, 2022

Conversation

radical
Copy link
Member

@radical radical commented Jun 13, 2022 •
edited
Loading

  • Fix an issue with stdout/stderr streams not being flushed correctly on nodejs.
  • Fix an issue with completely reading stdout/stderr for xharness
  • And remove dependence on xharness' std* streams, and instead use the emitted wasm-console.log, for non-browser cases
    • note: this is still correctly testing the app output, as xharness reads that, and writes to wasm-console.log
  • And copy sdk for workload testing only when running on CI.

Fixes #70675

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost assigned radical Jun 13, 2022
@ghost
Copy link

ghost commented Jun 13, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

… exiting

This is adding the fix from c361857 to
console template's js, which runs using node.

When the results xml is large, and we are writing the base64
representation in one line, node can exit before all the output gets
flushed out. This results in xharness getting an incomplete
STARTRESULTXML <len> <base64> ... with missing ENDRESULTXML, thus
no testResults.xml is generated.

This can be seen in the case of Microsoft.Extensions.Primitives.Tests
which has xml ~140KB, and System.Memory.Tests which has a xml ~13MB.

So, wait for the two streams to be flushed out, with a timeout of 3secs.

  • use the drain event only if stream.write('') returns false
Author: radical
Assignees: radical
Labels:

arch-wasm, area-Build-mono, test-failure
Milestone: -

@radical radical changed the title [wasm][nodejs] Ensure that stdout/stderr have been flushed out before… [wasm][nodejs] Fix console template running with node, to always flush the output stream before exit Jun 13, 2022
@radical radical mentioned this pull request Jun 16, 2022
Closed
@pavelsavara pavelsavara mentioned this pull request Jun 24, 2022
Merged
@pavelsavara
Copy link
Member

We will need to update this PR after #70746

@radical radical force-pushed the fix-wbt-node-app branch from b540827 to 64e316a Compare June 27, 2022 21:42
@radekdoulik radekdoulik mentioned this pull request Jun 28, 2022
Merged
@radical
Copy link
Member Author

radical commented Jun 28, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Jun 29, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Jun 29, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

radical added 4 commits June 29, 2022 01:12
@radical
[wasm][nodejs] Ensure that stdout/stderr have been flushed out before…
0640999 
… exiting

When the results xml is large, and we are writing the base64
representation in one line, `node` can exit before all the output gets
flushed out. This results in xharness getting an incomplete
`STARTRESULTXML <len> <base64> ... ` with missing `ENDRESULTXML`, thus
no `testResults.xml` is generated.

This can be seen in the case of `Microsoft.Extensions.Primitives.Tests`
which has xml ~140KB, and `System.Memory.Tests` which has a xml ~13MB.

So, wait for the two streams to be flushed out, with a timeout of 3secs.
@radical
[wasm] Wasm.Build.Tests: improve reading output from processes
ddc89af 
- Fix to call `WaitForExit()` once `WaitForExit(int)` returns, which
  ensures that all the async handlers have been run.

- Also, for non-browser xharness runs use the emitted `wasm-console.log`
  as the output, so we don't depend on xharness' stdout.
@radical
Wasm.Build.Tests: Run the crypto test only with browser. Other cases …
6dee11e 
…are covered in library tests
@radical
Enable WasmTemplateTests.ConsolePublishAndRun
43d6be7 
Fixes dotnet#70675
@radical radical force-pushed the fix-wbt-node-app branch from 284b1bb to 43d6be7 Compare June 29, 2022 05:12
@radical
Copy link
Member Author

radical commented Jun 29, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical radical changed the title [wasm][nodejs] Fix console template running with node, to always flush the output stream before exit [wasm] Wasm.Build.Tests - test fixes Jun 29, 2022
@radical
Wasm.Build.Tests: avoid unncessary copy when building locally
d37afef 
Copy sdk for testing workloads only on CI.
@radical
Copy link
Member Author

radical commented Jun 29, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical radical requested a review from lewing June 29, 2022 07:46
@radical radical requested review from kg, pavelsavara and maraf June 29, 2022 07:46
@radical radical marked this pull request as ready for review June 29, 2022 07:47
@radical radical requested a review from marek-safar as a code owner June 29, 2022 07:47
@radical radical requested a review from radekdoulik June 29, 2022 18:45
@radical radical changed the title [wasm] Wasm.Build.Tests - test fixes [wasm] Wasm.Build.Tests - fixes for tests failing on CI Jun 29, 2022
@radical
Copy link
Member Author

radical commented Jun 29, 2022

this has a timeout right now for flushing the streams, but maybe that should be removed?

kg
kg reviewed Jun 30, 2022
View reviewed changes
src/mono/wasm/test-main.js Outdated Show resolved Hide resolved
kg
kg requested changes Jun 30, 2022
View reviewed changes
Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

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

This looks fine to me other than the bits of feedback I provided, changes not mandatory

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 30, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 30, 2022
@radical
Copy link
Member Author

radical commented Jun 30, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical radical requested a review from kg June 30, 2022 04:36
@radical
Copy link
Member Author

radical commented Jun 30, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@radical
Copy link
Member Author

radical commented Jun 30, 2022

Chrome debugger test failure is intermittent, and unrelated.

@radical radical merged commit ce6d3df into dotnet:main Jun 30, 2022
@radical radical deleted the fix-wbt-node-app branch June 30, 2022 10:13
@ghost ghost locked as resolved and limited conversation to collaborators Jul 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kg kg kg approved these changes

@lewing lewing Awaiting requested review from lewing lewing is a code owner

@pavelsavara pavelsavara Awaiting requested review from pavelsavara pavelsavara is a code owner

@maraf maraf Awaiting requested review from maraf

@marek-safar marek-safar Awaiting requested review from marek-safar

@radekdoulik radekdoulik Awaiting requested review from radekdoulik

Assignees

@radical radical

Labels
arch-wasm WebAssembly architecture area-Build-mono test-failure
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[wasm] nodejs on windows doesn't flush stdout/stderr streams on exit - with the consolewasm template
3 participants
@radical @pavelsavara @kg