Skip to content

Conversation

@jozkee
Copy link
Member

@jozkee jozkee commented Jul 23, 2021

Follow-up to #56189 (comment) and to #56189 (comment).

I might need to add tests that prove the PlatformNotSupportedException added in #56189.
EDIT: CI did indeed caught these errors, the proper change now is to skip tests that use FileSystemWatcher from running in browser, that is made in 57bbe42.

@ghost
Copy link

ghost commented Jul 23, 2021

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

Issue Details

Follow-up to #56189 (comment).
I might need to add tests that prove the PlatformNotSupportedException added in #56189.

Author: Jozkee
Assignees: -
Labels:

area-Extensions-FileSystem

Milestone: -

@jozkee jozkee requested a review from lewing July 23, 2021 22:44
@jozkee jozkee changed the title Do not prevent Extensions.FileProviders tests from running in CI Do not prevent Extensions.FileProviders tests from running in browser Jul 23, 2021
@adamsitnik adamsitnik added this to the 6.0.0 milestone Jul 28, 2021
@lewing
Copy link
Member

lewing commented Aug 2, 2021

@jozkee jozkee requested a review from akoeplinger August 2, 2021 19:26
@jozkee jozkee marked this pull request as ready for review August 2, 2021 19:27
@jozkee jozkee force-pushed the fileprovider-browser-tests branch from a6d6483 to 0fe3738 Compare August 2, 2021 19:30
@jozkee jozkee closed this Aug 2, 2021
@jozkee jozkee reopened this Aug 2, 2021
@jozkee
Copy link
Member Author

jozkee commented Aug 2, 2021

Seems like CI doesn't want to be triggered by recent changes. cc @dotnet/runtime-infrastructure.

@ericstj
Copy link
Member

ericstj commented Aug 2, 2021

Builds are triggered but the GitHub badges aren't being updated. https://dnceng.visualstudio.com/public/_build?definitionId=686

Here's the build for this PR: https://dnceng.visualstudio.com/public/_build/results?buildId=1271949&view=results

…rgumentNullException in PhysicalFilesWatcher
@eerhardt
Copy link
Member

eerhardt commented Aug 4, 2021

Looks like the FileProviders tests are failing on wasm.

@jozkee
Copy link
Member Author

jozkee commented Aug 4, 2021

@eerhardt yup, one failure is related to 276b008, it seems that I also need to set UsePollingFileWatcher and UseActivePolling as well as nulling the FileSystemWatcher.

The other failures showing the Moq.CastleProxyFactory callstack seem to only be failing in browser AOT, perhaps is something related to how nuget packages are used in AOT? I wonder if we can just disable the tests for that specific case (and keep running them in regular browser).

[18:27:44] fail: [FAIL] Microsoft.Extensions.FileProviders.Physical.PollingWildCardChangeTokenTest.HasChanged_ReturnsFalseIfFilesDoNotChange
[18:27:44] info: System.ArgumentNullException : Value cannot be null. (Parameter 'method')
[18:27:44] info:    at Castle.DynamicProxy.Contributors.CompositeTypeContributor.ImplementMethod(MetaMethod , ClassEmitter , ProxyGenerationOptions , OverrideMethodDelegate ) in Castle.Core.dll:token 0x60002d5+0xc
[18:27:44] info:    at Castle.DynamicProxy.Contributors.CompositeTypeContributor.Generate(ClassEmitter , ProxyGenerationOptions ) in Castle.Core.dll:token 0x60002d0+0x1d
[18:27:44] info:    at Castle.DynamicProxy.Generators.ClassProxyGenerator.GenerateType(String , Type[] , INamingScope ) in Castle.Core.dll:token 0x60000f1+0x91
[18:27:44] info:    at Castle.DynamicProxy.Generators.ClassProxyGenerator.<>c__DisplayClass1_0.<GenerateCode>b__0(String , INamingScope ) in Castle.Core.dll:token 0x60000f5+0x0
[18:27:44] info:    at Castle.DynamicProxy.Generators.BaseProxyGenerator.<>c__DisplayClass33_0.<ObtainProxyType>b__0(CacheKey ) in Castle.Core.dll:token 0x60000ea+0x80
[18:27:44] info:    at Castle.Core.Internal.SynchronizedDictionary`2[[Castle.DynamicProxy.Generators.CacheKey, Castle.Core, Version=4.0.0.0, Culture=neutral, PublicKeyToken=407dd0808d44fbdc],[System.Type, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetOrAdd(CacheKey , Func`2 ) in Castle.Core.dll:token 0x600037f+0x57
[18:27:44] info:    at Moq.CastleProxyFactory.CreateProxy(Type , IInterceptor , Type[] , Object[] ) in Moq.dll:token 0x60000f1+0xa4
[18:27:44] info:    at System.Reflection.RuntimeMethodInfo.Invoke(Object , BindingFlags , Binder , Object[] , CultureInfo ) in System.Private.CoreLib.dll:token 0x6001d10+0x6a

@eerhardt
Copy link
Member

eerhardt commented Aug 4, 2021

For the Moq failures, the way we fix this in other tests is by using an xml file that tells the linker to preserve the types. See

https://github.com/dotnet/runtime/blob/main/eng/testing/ILLinkDescriptors/ILLink.Descriptors.Castle.xml

You need to include that in the tests, like we do elsewhere. For example:

<TrimmerRootDescriptor Include="$(ILLinkDescriptorsPath)ILLink.Descriptors.Castle.xml" />

@jozkee
Copy link
Member Author

jozkee commented Aug 4, 2021

@eerhardt ah nice, didn't know it was a known issue. I just pushed, so I expect CI to be clean this time.

@jozkee
Copy link
Member Author

jozkee commented Aug 4, 2021

@eerhardt @lewing is object equality in Browser vs other platforms different in any way? Some Assert.Equals are failing in CI and that's my first guess.
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-56257-merge-cba926854cbe49109d/Microsoft.Extensions.FileProviders.Physical.Tests/1/console.4a902b68.log?sv=2019-07-07&se=2021-08-24T16%3A32%3A23Z&sr=c&sp=rl&sig=5uJAuGCx3e2r6fIZeuynbVVWhUePF3i5tbjaOx4Zh%2Bw%3D

In the meantime I will try using Assert.Same to force reference equality since that's what the failing tests are meant to prove anyway.

@eerhardt
Copy link
Member

eerhardt commented Aug 4, 2021

is object equality in Browser vs other platforms different in any way?

Not that I know of.

I'm assuming this test is failing because on Browser we are using a polling watcher?

var token1 = provider.Watch(fileName);
var token2 = provider.Watch(fileName);
Assert.NotNull(token1);
Assert.NotNull(token2);
Assert.Equal(token2, token1);

@jozkee
Copy link
Member Author

jozkee commented Aug 4, 2021

@eerhardt you are right. I will just disable those tests for browser, thanks again.

@jozkee jozkee merged commit 94f8f0c into dotnet:main Aug 5, 2021
@jozkee jozkee deleted the fileprovider-browser-tests branch August 5, 2021 15:42
@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants