Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Adding support for custom ValidationResult (derived class) in CustomV…#35584

Merged
ajcvickers merged 4 commits intodotnet:masterfrom
Liero:master
Mar 4, 2019
Merged

Adding support for custom ValidationResult (derived class) in CustomV…#35584
ajcvickers merged 4 commits intodotnet:masterfrom
Liero:master

Conversation

@Liero
Copy link

@Liero Liero commented Feb 26, 2019

#33767

@Wraith2
Copy link
Contributor

Wraith2 commented Feb 26, 2019

/azp run corefx-ci (Windows NETFX_x86_Release)

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@Wraith2
Copy link
Contributor

Wraith2 commented Feb 27, 2019

I don't think the CI failure is caused by your change. It's the usual failure to upload to helix on a single step but i don't see an actual test failure. relevant log items are:

C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19126.3\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(67,5): error : Work item System.ComponentModel.Annotations.Tests in job 1cf89d74-cf63-4328-b82f-8791e5f48a51 has Failed with exit code 1 [D:\a\1\s\eng\sendtohelix.proj]
C:\Users\VssAdministrator\.nuget\packages\microsoft.dotnet.helix.sdk\2.0.0-beta.19126.3\tools\Microsoft.DotNet.Helix.Sdk.MultiQueue.targets(67,5): error : Job '1cf89d74-cf63-4328-b82f-8791e5f48a51' failed 1 out of 226855 tests. [D:\a\1\s\eng\sendtohelix.proj]

/cc @safern for confirmation that i'm reading that right.

If if is a false failure then you need to copy in the area owners so they can review, @lajones, @divega, @ajcvickers

@safern
Copy link
Member

safern commented Feb 27, 2019

This actually seems like a failure with this PR itself. If you look at the failure:
System.InvalidOperationException : The CustomValidationAttribute method 'ValidationMethodDerivedReturnTypeReturnsSomeError' in type 'CustomValidator' must return System.ComponentModel.DataAnnotations.ValidationResult. Use System.ComponentModel.DataAnnotations.ValidationResult.Success to represent success.

It is failing directly here, on the added CustomValidator. It makes sense, since NETFX doesn't have this support: https://github.com/dotnet/corefx/pull/35584/files#diff-5fb1c2fa77569a06289a2ffca62c378aR246

So the test will need to be conditioned to only run on netcoreapp.

To skip the test entirely for NETFX, we can add an attribute to https://github.com/dotnet/corefx/pull/35584/files#diff-80b5d2f008710e7c2a1b943f176c98dbR183:
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Reason to skip it")]

@Wraith2
Copy link
Contributor

Wraith2 commented Feb 27, 2019

Ok, I can't work out where you're seeing that, perhaps i'm missing something obvious. If I click though the netfx coreci failure Details link above, then click the errors/warnings link and I get to here:
capture
which only allows me to see the helix upload step is failing and the two messages above, I can't find the errors you're seeing anywhere.

@safern
Copy link
Member

safern commented Feb 27, 2019

If you click on the Send to Helix step to open the logs, then there will be a URL within the log, something like:
Results will be available from https://mc.dot.net/#/user/dotnet-bot/pr~2Fdotnet~2Fcorefx~2Frefs~2Fpull~2F35584~2Fmerge/test~2Ffunctional~2Fcli~2F/20190227.1

image

I have a draft on the docs already. Should send a PR today 😄

@Wraith2
Copy link
Contributor

Wraith2 commented Feb 27, 2019

Oh! ok, so you follow the links through all the fancy stuff to get a link that you have to copy paste out into a totally different app. That's not quite as obvious as it could perhaps be. Thanks for the info though, that'll stop me copying you into issues so you can read the log for me 😀

@safern
Copy link
Member

safern commented Feb 27, 2019

Oh! ok, so you follow the links through all the fancy stuff to get a link that you have to copy paste out into a totally different app. That's not quite as obvious as it could perhaps be. Thanks for the info though, that'll stop me copying you into issues so you can read the log for me 😀

You can also do CTRL+Click and you don't have to copy the link from the logs. We're definitely working on improving that experience since it is painful, but we're limited now since we depend from azure devops fixes we requested, but it should improve on time 😄

@Liero
Copy link
Author

Liero commented Feb 28, 2019

I'm not sure I follow why it is failing in .NET Framework..

@safern
Copy link
Member

safern commented Feb 28, 2019

I'm not sure I follow why it is failing in .NET Framework..

Because .NET Framework doesn't support custom ValidationResult, because the support you're adding here is for .NET Core only, bits for .NET Core and UAP from this repo. .NET Framework is another code base and most likely it will not get this back-ported.

@Liero
Copy link
Author

Liero commented Mar 1, 2019

Ok, I will fix the tests. However, is there some article that explains how this works, how to test in on local machine etc? Some guidance for community. Or I'm left only with the readmes in this repo?

@Wraith2
Copy link
Contributor

Wraith2 commented Mar 1, 2019

Most of what you need it in the documentation folder in various places. There's also a doc just being added which covers some of the newer aspects #35672 (comment)

@ajcvickers ajcvickers merged commit cb726dd into dotnet:master Mar 4, 2019
@ajcvickers
Copy link

Thanks!

@karelz karelz added this to the 3.0 milestone Mar 18, 2019
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.

7 participants