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

[browser] enable JsImportTaskTypes test #88728

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

pavelsavara
Copy link
Member

@pavelsavara pavelsavara commented Jul 12, 2023

Fixes #77334
I can't reproduce it anymore

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm labels Jul 12, 2023
@pavelsavara pavelsavara added this to the 8.0.0 milestone Jul 12, 2023
@pavelsavara pavelsavara requested a review from maraf July 12, 2023 11:24
@pavelsavara pavelsavara requested a review from lewing as a code owner July 12, 2023 11:24
@pavelsavara pavelsavara self-assigned this Jul 12, 2023
@ghost
Copy link

ghost commented Jul 12, 2023

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

Issue Details

I can't reproduce #77334 anymore

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript, os-browser

Milestone: 8.0.0

Assert.Same(e, await JavaScriptTestHelper.echopromise_Exception(e));
Assert.Same(j, await JavaScriptTestHelper.echopromise_JSObject(j));
GC.Collect();
await Task.Delay(10);
Copy link
Member

Choose a reason for hiding this comment

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

Just curious. Is an explicit GC.Collect needed if we have a delay here, that should effectively yield?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this will yield to browser main loop, which I hope would give it a chance to run JS GC.

Copy link
Member

Choose a reason for hiding this comment

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

Delay(1) will also work (only 0 completes immediately) I think Task.Yield() should work as well

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW: on V8 the setTimeout value is always ignored.

@pavelsavara
Copy link
Member Author

It also passed on CI, Log

I don't know what fixed it, so it may appear again when running regularly on CI, but that would be better than not testing it at all.

@pavelsavara pavelsavara merged commit 2e7f0dc into dotnet:main Jul 12, 2023
104 of 109 checks passed
@pavelsavara
Copy link
Member Author

It re-appears here I will roll this back

pavelsavara added a commit to pavelsavara/runtime that referenced this pull request Jul 13, 2023
pavelsavara added a commit that referenced this pull request Jul 13, 2023
dotnet-bot pushed a commit to dotnet/dotnet that referenced this pull request Jul 14, 2023
Diff: https://github.com/dotnet/runtime/compare/e4c5b3eda1952af05a4592fbe093a2e396d1902e..ae99bb2e7a0a5db7c2bff129322965fd3c6e820d

From: dotnet/runtime@e4c5b3e
To: dotnet/runtime@ae99bb2

Commits:
  - [mono][tests] Attempt to enable test (#88722)
    dotnet/runtime@c6e8726
  - [LoongArch64] Implement Inline TLS field access for LoongArch64. (#88819)
    dotnet/runtime@76a7d1e
  - revert active issue dotnet/runtime#88728 (#88831)
    dotnet/runtime@2f853d1
  - Skip RC2 encrypted PKCS12 files on Android for iteration counting
    dotnet/runtime@dedaf46
  - JSON: Add support for {ReadOnly}Memory<T> (#88713)
    dotnet/runtime@4d5d2dc
  - [browser] Migrate more Blazor features, prepare JavaScript API for Blazor cleanup (#87959)
    dotnet/runtime@acccc01
  - Ensure DOTNET_MaxVectorTBitwidth is interpreted as a decimal based input, not hexadecimal (#88761)
    dotnet/runtime@33e0669
  - Add Keyed Services Support to Dependency Injection (#87183)
    dotnet/runtime@138cb59
  - Deduplicate diagnostics emitted for managed->unmanaged and unmanaged->managed stubs (#88803)
    dotnet/runtime@ff7157b
  - Don't generate Unmanaged-to-Managed stubs with diagnostics (#88679)
    dotnet/runtime@e0acb9d
  - Ensure Vector256.Dot produces a V256 result (#88712)
    dotnet/runtime@620bd3e
  - Do not say the wrong default behavior for blittable arrays in ComInterfaceGenerator warning (#88212)
    dotnet/runtime@d361507
  - [PERF] Explicitly set dotnet root when building tools (#88801)
    dotnet/runtime@58cd4d2
  - Update enum value and assert (#88830)
    dotnet/runtime@563a549
  - Add EnumBuilder implementation and other changes (#88503)
    dotnet/runtime@3eb6eef
  - [libs][Unix] Fix UTC alias lookup (#88641)
    dotnet/runtime@218d2ef
  - HttpClientHandler request metrics (#87319)
    dotnet/runtime@28a4c95
  - Add missing sections in implicit machine.config (#88553)
    dotnet/runtime@62d6335
  - [JIT] Added BEGIN and END anchors for disasm output (#88782)
    dotnet/runtime@36e32f8
  - [mono][tests] Fix deadlock in pinvoke detach test (#88855)
    dotnet/runtime@31b1fd4
  [... commit list trimmed ...]

[[ commit created by automation ]]
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
@pavelsavara pavelsavara deleted the browser_disposed_object branch September 2, 2024 15:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[wasm] S.R.IS.JavaScript.Tests.JSImportExportTest.JsImportTaskTypes
3 participants