Update SharedInfrastructure for Xunit V3#2854
Conversation
|
I started looking into it. Gonna take longer. This is not a small change. |
# Conflicts: # shared-infrastructure
1bb255f to
29ff145
Compare
|
You’re a brave man! XUnit versioning is a shambles. |
|
@JimBobSquarePants Just small changes then it should work. But those changes are 🤮 Will have to clean it up. I just saw that there are some unrelated changes. |
Breaking changes addressed: - Update xunit.v3 to 3.2.2 and xunit.runner.visualstudio to 3.1.5 - Remove Microsoft.DotNet.XUnitExtensions (conflicts with xunit.v3 via xunit.core v2) - Replace ConditionalFact/ConditionalTheory with Fact/Theory SkipUnless/SkipWhen/SkipType - Remove 'using Xunit.Abstractions' (ITestOutputHelper now in Xunit namespace globally) - Fix TestFrameworkAttribute: string args → typeof(T) form - Fix XunitTestFramework ctor: no longer takes IMessageSink - Fix BeforeAfterTestAttribute Before/After: added IXunitTest parameter, use Xunit.v3 ns - Fix DataAttribute: new GetData signature (DisposalTracker), add SupportsDiscoveryEnumeration() - Fix IXunitSerializable/IXunitSerializationInfo: add 'using Xunit.Sdk' to all consumers - Fix BasicSerializer: implement new IXunitSerializationInfo.GetValue(string) returning object? Store type info in dump format (key:TypeAQN:value) to enable typed reconstruction - Fix TheoryData<T> CS0121 ambiguity on C# 12/net8.0: add explicit casts (T)value Affects ExifValuesTests, PixelOperationsTests, QuantizerTests, DitherTests, TestImageProviderTests, RotateTests, L8Tests, La16Tests - Fix PngEncoderTests: TheoryData<T> now yields TheoryDataRow<T>, not object[]
1113a0f to
d50cba9
Compare
when used in nameof it is not allowed to be a filed it must be a property
1233a91 to
29ff120
Compare
|
@JimBobSquarePants so i finale succeeded. If you want you can have a look |
|
|
||
| // TODO: Figure out cancellation failures on Linux | ||
| [ConditionalTheory(typeof(TestEnvironment), nameof(TestEnvironment.IsWindows))] | ||
| [Theory(Skip = "Skipped on non-Windows platforms", SkipType = typeof(TestEnvironment), SkipUnless = nameof(TestEnvironment.IsWindows))] |
There was a problem hiding this comment.
[ConditionalTheory] comes from Arcade doesn't it. Can we write our own version? Seems less verbose.
There was a problem hiding this comment.
I can look into it. Arcade is strange. You use a package which is not findable on Nuget
There was a problem hiding this comment.
Yeah it’s from a runtime feed we add via the submodule. Has RemoteExecutor also.
There was a problem hiding this comment.
I can add it. Where would you want me to put it? TestUtilities.XUnit.Attributes ?
| { | ||
| string[] kv = s.Split(Separator); | ||
| this.map[kv[0]] = kv[1]; | ||
| // Format: key:TypeAQN:value (3 parts max to preserve ':' inside value) |
There was a problem hiding this comment.
I'll need you to explain all this I'm afraid.
There was a problem hiding this comment.
The old interface did pass the type in the Get. That's why it was saved internally as key:value.
With XunitV3 we do not get that info with the get method that's why we now save it in the dictionary. So we now have 3 parts and not 3 parts like before
fac4787 to
7e89cf2
Compare
f6d32f8 to
3a434f7
Compare
| GCMemoryInfo memInfo = GC.GetGCMemoryInfo(); | ||
| int highLoadThreshold = (int)(memInfo.HighMemoryLoadThresholdBytes / oneMb); | ||
| highLoadThreshold = (int)(trimSettings.HighPressureThresholdRate * highLoadThreshold); | ||
| highLoadThreshold = (int)( |
There was a problem hiding this comment.
Formatting like this is weird. Can you please look in the diff for other odd changes like this and revert. I've spotted a few.
| { | ||
| public static readonly TheoryData<byte> LuminanceData | ||
| = new() { 0, 1, 2, 3, 5, 13, 31, 71, 73, 79, 83, 109, 127, 128, 131, 199, 250, 251, 254, 255 }; | ||
| = new() { (byte)0, (byte)1, (byte)2, (byte)3, (byte)5, (byte)13, (byte)31, (byte)71, (byte)73, (byte)79, (byte)83, (byte)109, (byte)127, (byte)128, (byte)131, (byte)199, (byte)250, (byte)251, (byte)254, (byte)255 }; |
There was a problem hiding this comment.
And we definitely need this new casting?
There was a problem hiding this comment.
Does casting the first entry work? (byte)0
| new WebSafePaletteQuantizer(OrderedDitherOptions), | ||
| new WernerPaletteQuantizer(OrderedDitherOptions), | ||
| new WuQuantizer(OrderedDitherOptions) | ||
| (IQuantizer)KnownQuantizers.Hexadecatree, |
There was a problem hiding this comment.
Same here. Lots of new casting
| { | ||
| addedRows = memberItems.Select(x => new[] { x }); | ||
| } | ||
| if (x is ITheoryDataRow row) return row.GetData(); |
There was a problem hiding this comment.
There should be an editorconfig setting for this.
Another thing is formatting.
Should we work onto an ci check and tooling that formats the code? Or do you want it handcrafted.
I made good experience with csharpier. It's opinionated. At first a bit strange but after getting used to it 👌 and no more formatting discussions or problems
There was a problem hiding this comment.
Should we work onto an ci check and tooling that formats the code? Or do you want it handcrafted.
No. I do not want to ever rely on automation for code formatting. I want people to be aware of the styles and standards.
There should be an editorconfig setting for this.
csharp_prefer_braces is already present as a warning. We just don't have them as errors in the tests.
| [Fact] | ||
| public void SolutionDirectoryFullPath() | ||
| => this.CheckPath(TestEnvironment.SolutionDirectoryFullPath); | ||
| public void SolutionDirectoryFullPath() => |
There was a problem hiding this comment.
Odd formatting changes
| public void GetReferenceOutputFileName() | ||
| { | ||
| string actual = Path.Combine(TestEnvironment.ActualOutputDirectoryFullPath, @"foo\bar\lol.jpeg"); | ||
| string actual = Path.Combine( |
There was a problem hiding this comment.
No need to wrap here.
| [InlineData("lol/Baz.gif", typeof(GifDecoder))] | ||
| [InlineData("lol/foobar.webp", typeof(WebpDecoder))] | ||
| public void GetReferenceDecoder_ReturnsCorrectDecoders_Windows(string fileName, Type expectedDecoderType) | ||
| public void GetReferenceDecoder_ReturnsCorrectDecoders_Windows( |
| CalleeType = calleeType; | ||
| ConditionMemberNames = conditionMemberNames; | ||
| string skipReason = ConditionalTestDiscoverer.EvaluateSkipConditions(calleeType, conditionMemberNames); | ||
| if (skipReason != null) |
| Skip = skipReason; | ||
| } | ||
|
|
||
| [Obsolete( |
| List<string> falseConditions = new(conditionMemberNames.Length); | ||
| foreach (string entry in conditionMemberNames) | ||
| { | ||
| if (string.IsNullOrWhiteSpace(entry)) |
| conditionMemberNames | ||
| ); | ||
| if (skipReason != null) | ||
| Skip = skipReason; |
| Skip = skipReason; | ||
| } | ||
|
|
||
| [Obsolete( |
| public DynamicallyAccessedMemberTypes MemberTypes { get; } | ||
| } | ||
|
|
||
| internal enum DynamicallyAccessedMemberTypes |
There was a problem hiding this comment.
This should be available to you without copying.
| if (directory == null) | ||
| { | ||
| throw new DirectoryNotFoundException($"Unable to find ImageSharp solution directory from {TestAssemblyFile}!"); | ||
| throw new DirectoryNotFoundException( |
| $"Unable to find ImageSharp solution directory from {TestAssemblyFile} because of {ex.GetType().Name}!", | ||
| ex); | ||
| ex | ||
| ); |
There was a problem hiding this comment.
I've seen this change in a few places now and it violates StyleCop
| internal static bool IsMono => Type.GetType("Mono.Runtime") != null; // https://stackoverflow.com/a/721194 | ||
|
|
||
| internal static bool IsWindows => RuntimeInformation.IsOSPlatform(OSPlatform.Windows); | ||
| public static bool IsWindows => RuntimeInformation.IsOSPlatform(OSPlatform.Windows); |
There was a problem hiding this comment.
Why is this public and no others?
There was a problem hiding this comment.
This was needed for using plain fact. This can probably switched back to internal
|
|
||
| string remoteExecutorConfigPath = | ||
| Path.Combine(TestAssemblyFile.DirectoryName, "Microsoft.DotNet.RemoteExecutor.exe.config"); | ||
| string remoteExecutorConfigPath = Path.Combine( |



Submodule points to my fork for iterating faster. When this works, I will create a mr in the SharedInfrastructure project