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

Fix information flow about fingerprinting dotnet files in WBT #112859

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ilonatommy
Copy link
Member

@ilonatommy ilonatommy commented Feb 24, 2025

Fixes #112814.

Tested on #112730. We were missing a correct flow of WasmFingerprintDotnetJs value in WBT. The test doesn't have to be disabled with this fix.

The logging that was commented out in the method that was failing was actually useful, I would keep it.

@ilonatommy ilonatommy added arch-wasm WebAssembly architecture os-browser Browser variant of arch-wasm labels Feb 24, 2025
@ilonatommy ilonatommy added this to the 10.0.0 milestone Feb 24, 2025
@ilonatommy ilonatommy self-assigned this Feb 24, 2025
@Copilot Copilot bot review requested due to automatic review settings February 24, 2025 13:26
@ilonatommy ilonatommy requested a review from maraf as a code owner February 24, 2025 13:26

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/mono/wasm/Wasm.Build.Tests/ProjectProviderBase.cs:122

  • Uncommenting this debug logging line may clutter the test output during normal runs. Consider gating the logging behind a verbose condition or a flag to keep output clean by default.
_testOutput.WriteLine("Comparing {expectedFilename} with {actualFile}, expectFingerprintOnDotnetJs: {expectFingerprintOnDotnetJs}, expectFingerprint: {expectFingerprint}");

src/mono/wasm/Wasm.Build.Tests/Common/EnvironmentVariables.cs:26

  • The environment variable key was changed from 'WASM_FINGERPRINT_DOTNET_JS' to 'USE_DOTNET_FINGERPRINTING_FOR_TESTS'; please ensure that all test configurations and dependent code are updated to reflect this change.
internal static readonly bool UseFingerprintingDotnetJS    = Environment.GetEnvironmentVariable("USE_DOTNET_FINGERPRINTING_FOR_TESTS") is "true";
Copy link
Member

@lewing lewing left a comment

Choose a reason for hiding this comment

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

did you mean to leave the test output line in the pr?

@ilonatommy
Copy link
Member Author

Yes, I did. Do you think it's too noisy? If it was there, it would have shortened the investigation. I could also collect the text from 122 and print only in line 152, when we for sure throw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly architecture area-Build-mono os-browser Browser variant of arch-wasm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Evaluate fingerprinting test data in Blazor.BuildPublishTests.DefaultTemplate_CheckFingerprinting
3 participants