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.

Sign up for GitHub

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

Jump to bottom

Restore write-resource-to-text tests #4830

Merged
merged 8 commits into from
Oct 24, 2019
Merged

Restore write-resource-to-text tests #4830

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9aa42bb
Restore write-resource-to-text tests
rainersigwald Oct 18, 2019
86c74a5
Disable InvalidReference test on Core
rainersigwald Oct 18, 2019
3a6da3e
Error when serializing resources to text
rainersigwald Oct 18, 2019
a89802a
Support TestEnvironment in resx tests
rainersigwald Oct 21, 2019
bc04a15
Failure-to-read-resources test is desktop only
rainersigwald Oct 21, 2019
1de44ef
Allow core-only tests
rainersigwald Oct 21, 2019
60f260c
Localized error for can't-read-that-from-Core
rainersigwald Oct 21, 2019
2c99582
Remove redundant delete in WriteTestResX
rainersigwald Oct 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Directory.Build.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
<XunitOptions Condition="'$(OsEnvironment)'=='windows' and '$(MonoBuild)' == 'true'">$(XunitOptions) -notrait category=mono-windows-failing</XunitOptions>

<XunitOptions Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">$(XunitOptions) -notrait category=nonnetcoreapptests</XunitOptions>
<XunitOptions Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework'">$(XunitOptions) -notrait category=nonnetfxtests</XunitOptions>

<XunitOptions>$(XunitOptions) -notrait category=failing</XunitOptions>

Expand Down
60 changes: 42 additions & 18 deletions src/Tasks.UnitTests/ResourceHandling/GenerateResource_Tests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1713,11 +1713,7 @@ public void DuplicateResourceNames()
/// <summary>
/// Non-string resource with text output
/// </summary>
#if RUNTIME_TYPE_NETCORE
[Fact (Skip = "https://github.com/Microsoft/msbuild/issues/308")]
#else
[Fact]
#endif
public void UnsupportedTextType()
{
string bitmap = Utilities.CreateWorldsSmallestBitmap();
Expand Down Expand Up @@ -1776,11 +1772,8 @@ public void InvalidStateFile()
/// <summary>
/// Cause failures in ResourceReader
/// </summary>
#if RUNTIME_TYPE_NETCORE
[Fact (Skip = "https://github.com/Microsoft/msbuild/issues/308")]
#else
[Fact]
#endif
[SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp, ".NET Core MSBuild doesn't try to read binary input resources")]
public void FailedResourceReader()
{
GenerateResource t = Utilities.CreateTask(_output);
Expand All @@ -1807,6 +1800,28 @@ public void FailedResourceReader()
File.Delete(item.ItemSpec);
}

[Theory]
[InlineData(".resources")]
[InlineData(".dll")]
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "This error is .NET Core only")]
public void ResourceReaderRejectsNonCoreCompatFormats(string inputExtension)
{
using var env = TestEnvironment.Create(_output);

GenerateResource t = Utilities.CreateTask(_output);
t.StateFile = new TaskItem(env.GetTempFile(".cache").Path);

// file contents aren't required since the extension is checked first
var resourcesFile = env.CreateFile(inputExtension).Path;

t.Sources = new ITaskItem[] { new TaskItem(resourcesFile) };
t.OutputResources = new ITaskItem[] { new TaskItem(env.GetTempFile(".resources").Path) };

t.Execute().ShouldBeFalse();

Utilities.AssertLogContains(t, "MSB3824");
}

/// <summary>
/// Invalid STR Class name
/// </summary>
Expand Down Expand Up @@ -1844,11 +1859,9 @@ public void FailedSTRProperty()
/// <summary>
/// Reference passed in that can't be loaded should error
/// </summary>
#if RUNTIME_TYPE_NETCORE
[Fact (Skip = "https://github.com/Microsoft/msbuild/issues/308")]
#else
[Fact]
#endif
[SkipOnTargetFramework(TargetFrameworkMonikers.Netcoreapp,
reason: ".NET Core MSBuild doesn't load refs so it pushes this failure to runtime")]
public void InvalidReference()
{
string txtFile = null;
Expand Down Expand Up @@ -3700,7 +3713,7 @@ public static string GetTestResXContent(bool useType, string linkedBitmap, strin
/// <param name="useType">Indicates whether to include an enum to test type-specific resource encoding with assembly references</param>
/// <param name="linkedBitmap">The name of a linked-in bitmap. use 'null' for no bitmap.</param>
/// <returns>The name of the resx file</returns>
public static string WriteTestResX(bool useType, string linkedBitmap, string extraToken, string resxFileToWrite = null)
public static string WriteTestResX(bool useType, string linkedBitmap, string extraToken, string resxFileToWrite = null, TestEnvironment env = null)
{
return WriteTestResX(useType, linkedBitmap, extraToken, useInvalidType: false, resxFileToWrite:resxFileToWrite);
}
Expand All @@ -3711,16 +3724,27 @@ public static string WriteTestResX(bool useType, string linkedBitmap, string ext
/// <param name="useType">Indicates whether to include an enum to test type-specific resource encoding with assembly references</param>
/// <param name="linkedBitmap">The name of a linked-in bitmap. use 'null' for no bitmap.</param>
/// <returns>The name of the resx file</returns>
public static string WriteTestResX(bool useType, string linkedBitmap, string extraToken, bool useInvalidType, string resxFileToWrite = null)
public static string WriteTestResX(bool useType, string linkedBitmap, string extraToken, bool useInvalidType, string resxFileToWrite = null, TestEnvironment env = null)
{
string resgenFile = resxFileToWrite;
if (string.IsNullOrEmpty(resgenFile))

string contents = GetTestResXContent(useType, linkedBitmap, extraToken, useInvalidType);

if (env == null)
{
if (string.IsNullOrEmpty(resgenFile))
{
resgenFile = GetTempFileName(".resx");
File.Delete(resgenFile);
Copy link
Member

Choose a reason for hiding this comment

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

(nit) Is this line necessary? According to the File.WriteAllText documentation, even if the file did exist it would be overwritten

Copy link
Member Author

Choose a reason for hiding this comment

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

It's preexisting but no, I don't think it is. I'll remove.

}

File.WriteAllText(resgenFile, contents);
}
else
{
resgenFile = GetTempFileName(".resx");
File.Delete(resgenFile);
resgenFile = env.CreateFile(".resx", contents).Path;
}

File.WriteAllText(resgenFile, GetTestResXContent(useType, linkedBitmap, extraToken, useInvalidType));
return resgenFile;
}

Expand Down
29 changes: 21 additions & 8 deletions src/Tasks/GenerateResource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2603,6 +2603,12 @@ private bool ProcessFile(string inFile, string outFileOrDir)
{
ReadResources(inFile, _useSourcePath, outFileOrDir);
}
catch (InputFormatNotSupportedException)
{
_logger.LogErrorWithCodeFromResources(null, FileUtilities.GetFullPathNoThrow(inFile), 0, 0, 0, 0,
"GenerateResource.CoreSupportsLimitedScenarios");
return false;
}
catch (MSBuildResXException msbuildResXException)
{
_logger.LogErrorWithCodeFromResources(null, FileUtilities.GetFullPathNoThrow(inFile), 0, 0, 0, 0,
Expand Down Expand Up @@ -2988,7 +2994,7 @@ private void ReadResources(String filename, bool shouldUseSourcePath, String out
#if FEATURE_ASSEMBLY_LOADFROM
ReadAssemblyResources(filename, outFileOrDir);
#else
_logger.LogError("Reading resources from Assembly not supported on .NET Core MSBuild");
throw new InputFormatNotSupportedException("Reading resources from Assembly not supported on .NET Core MSBuild");
Copy link
Member

Choose a reason for hiding this comment

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

Why is it advantageous to throw instead of just logging an error? Are you trying to reduce duplicate output?

Copy link
Member Author

Choose a reason for hiding this comment

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

Logging an error here is nonfatal, so the experience with going through this codepath was:

  1. This error.
  2. Some other odd output when trying to move on.
  3. Another fatal error that didn't directly indicate the problem.

Since there were already exception-based error paths in ProcessFile I opted to add another rather than change everything to error returns.

#endif
}
else
Expand Down Expand Up @@ -3046,10 +3052,10 @@ private void ReadResources(String filename, bool shouldUseSourcePath, String out
case Format.Binary:
#if FEATURE_RESX_RESOURCE_READER
ReadResources(reader, new ResourceReader(filename), filename); // closes reader for us
break;
#else
_logger.LogError("ResGen.exe not supported on .NET Core MSBuild");
throw new InputFormatNotSupportedException("Reading resources from binary .resources not supported on .NET Core MSBuild");
#endif
break;

default:
// We should never get here, we've already checked the format
Expand Down Expand Up @@ -3387,8 +3393,13 @@ private bool HaveSystemResourcesExtensionsReference

PopulateAssemblyNames();

foreach (var assemblyName in _assemblyNames)
foreach (AssemblyNameExtension assemblyName in _assemblyNames)
{
if (assemblyName == null)
Copy link
Member

Choose a reason for hiding this comment

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

Why would they be null?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what happens if an assembly name can't be extracted from a reference for some reason in PopulateAssemblyNames(). The other codepaths were robust to this situation but this newly-added one wasn't (until running a test revealed the error).

{
continue;
}

if (string.Equals(assemblyName.Name, "System.Resources.Extensions", StringComparison.OrdinalIgnoreCase))
{
_haveSystemResourcesExtensionsReference = true;
Expand Down Expand Up @@ -3761,14 +3772,16 @@ private void WriteTextResources(ReaderInfo reader, String fileName)
{
using (StreamWriter writer = FileUtilities.OpenWrite(fileName, false, Encoding.UTF8))
{
foreach (LiveObjectResource entry in reader.resources)
foreach (IResource resource in reader.resources)
{
String key = entry.Name;
Object v = entry.Value;
LiveObjectResource entry = resource as LiveObjectResource;

String key = entry?.Name;
Object v = entry?.Value;
String value = v as String;
if (value == null)
{
_logger.LogErrorWithCodeFromResources(null, fileName, 0, 0, 0, 0, "GenerateResource.OnlyStringsSupported", key, v.GetType().FullName);
_logger.LogErrorWithCodeFromResources(null, fileName, 0, 0, 0, 0, "GenerateResource.OnlyStringsSupported", key, v?.GetType().FullName);
}
else
{
Expand Down
28 changes: 28 additions & 0 deletions src/Tasks/ResourceHandling/InputFormatNotSupportedException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) Microsoft. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Runtime.Serialization;

namespace Microsoft.Build.Tasks.ResourceHandling
{
[Serializable]
internal class InputFormatNotSupportedException : Exception
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you can't use FormatException or ArgumentException instead of making your own (empty) new type of exception?

Copy link
Member Author

Choose a reason for hiding this comment

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

Having a custom exception type makes detecting this error condition easy using a catch-specific-type block.

Copy link
Member

Choose a reason for hiding this comment

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

Tracing how it's currently caught, I believe a FormatException will still have the exact same effect.

Also, I don't see anywhere that you try to catch it; do you have plans for adding something like that in the future? If not, you're breaking something that could otherwise be caught by an ArgumentException, failing more cleanly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe a FormatException will still have the exact same effect.

Since it's a standard exception type, other code could insert a FormatException that would then be inadvertently caught and handled as this specific problem, rather than handled at another layer.

Also, I don't see anywhere that you try to catch it

https://github.com/microsoft/msbuild/pull/4830/files#diff-1d2aacce8c88cda3235c1659ea67e164R2606-R2611

If not, you're breaking something that could otherwise be caught by an ArgumentException, failing more cleanly.

This is harder to distinguish since other places could throw ArgumentException inside the big ProcessFile catch block.

{
public InputFormatNotSupportedException()
{
}

public InputFormatNotSupportedException(string message) : base(message)
{
}

public InputFormatNotSupportedException(string message, Exception innerException) : base(message, innerException)
{
}

protected InputFormatNotSupportedException(SerializationInfo info, StreamingContext context) : base(info, context)
{
}
}
}
4 changes: 4 additions & 0 deletions src/Tasks/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1130,6 +1130,10 @@
<value>MSB3823: Non-string resources require the property GenerateResourceUsePreserializedResources to be set to true.</value>
<comment>{StrBegin="MSB3823: "}</comment>
</data>
<data name="GenerateResource.CoreSupportsLimitedScenarios">
<value>MSB3824: In order to build with .NET Core, resource inputs must be in .txt or .resx format.</value>
<comment>{StrBegin="MSB3824: "}</comment>
</data>


<!--
Expand Down
5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.cs.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.de.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.en.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.es.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.fr.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.it.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.ja.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.ko.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.pl.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.pt-BR.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.ru.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Tasks/Resources/xlf/Strings.tr.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading