Fix ParseVBErrorOrWarning by Youssef1313 · Pull Request #47833 · dotnet/roslyn

Youssef1313 · 2020-09-18T14:50:23Z

Not sure how/where to write tests for this. I was also unable to test this locally due to getting some unrelated exception (#47562 (comment)).

src/Compilers/Core/MSBuildTask/Vbc.cs

adambiser · 2020-09-18T20:02:45Z

Mightn't the error description contain parentheses? If so, using LastIndexOf would grab it instead.
I wonder whether it would be better to use IndexOf("):") since that combination isn't part of a valid path, so the first occurrence should be after the line number.

Youssef1313 · 2020-09-18T20:23:01Z

@adambiser Yeah that's correct. Fixed.

333fred

LGTM (commit 2). @jaredpar, can you take a second look?

AlekseyTs · 2020-09-24T17:45:48Z

I think we need to find a way to test the change.

Youssef1313 · 2020-09-24T18:12:06Z

@AlekseyTs I agree, but I'm unfortunately unaware of how to test this.

jaredpar · 2020-12-10T19:42:20Z

src/Compilers/Core/MSBuildTask/Vbc.cs

        /// <param name="singleLine">The line to parse</param>
        /// <param name="messageImportance">The MessageImportance to use when reporting the error.</param>
-        internal void ParseVBErrorOrWarning(string singleLine, MessageImportance messageImportance)
+        private void ParseVBErrorOrWarning(string singleLine, MessageImportance messageImportance)


Agree in general that it's hard to test the various Tasks because it becomes more of an integration test with MSBuild. In this particular case though we're effectively altering how VB error messages are parsed in the task. I think we could factor this method a bit, by either adding a static method or by just newing up the Task in test and setting the properies, and use that to validate this change works (and stays correct going forward).

adambiser · 2020-12-10T20:28:41Z

There is a pull request on Youssef1313:patch-7 that has tests and fixes the issue. The fix here does not parse what comes from vbc correctly (should be a space between the end parenthesis and the colon). It has been waiting for a couple of weeks. Should I close it and start a separate pull request for this issue?

333fred · 2020-12-10T21:52:14Z

It's possible that @Youssef1313 didn't know the request was there?

adambiser · 2020-12-10T22:02:52Z

Possibly, but I thought that sending a pull request also sent a notification.

333fred · 2020-12-10T23:01:06Z

Possibly, but I thought that sending a pull request also sent a notification.

On forks, I believe notifications are actually off by default.

adambiser · 2020-12-10T23:12:06Z

OK. I had forked from his fork so I thought he would get notification of the pull request to his repo.

Verifies the text used for Microsoft.Build.Tasks.CodeAnalysis.UnitTests.CheckErrorAndWarningParsing.

ErrorLoggerEngine was added to capture the parse results as produced by Microsoft.Build.Shared.EventArgsFormatting.

Youssef1313

Thanks @adambiser. That really helped a lot. And sorry for the delay, I didn't get notified of your PR on my fork.

Youssef1313 · 2020-12-11T03:41:41Z

src/Compilers/Core/MSBuildTaskTests/TestUtilities/ErrorLoggerEngine.cs

+        public void LogCustomEvent(CustomBuildEventArgs eventArgs)
+        {
+        }
+
+        public void LogMessageEvent(BuildMessageEventArgs eventArgs)
+        {
+        }


Should throw NotImplementedException instead?

And thank you, too. I had assumed you would get a notification since I had forked from you instead of the main project. Figured other things were filling your time, as is easy to happen nowadays. But now I know.

As for NotImplementedException, I don't think so. I added the class solely to get to the warning and error output, though I guess given the class's limited usage, throwing NotImplementedException in both probably won't affect things.
I also was unaware of how to test this and ended up with two. Not sure whether they will approve of the tests or not though.

333fred · 2020-12-11T20:27:21Z

src/Compilers/Core/MSBuildTaskTests/TestUtilities/ErrorLoggerEngine.cs

+            _formatErrorMethod = formattingClass.GetMethod("FormatEventMessage", BindingFlags.Static | BindingFlags.NonPublic, null, CallingConventions.Any,
+                new Type[] { typeof(BuildErrorEventArgs) }, null) ?? throw new Exception("Could not find FormatEventMessage(BuildErrorEventArgs).");
+            _formatWarningMethod = formattingClass.GetMethod("FormatEventMessage", BindingFlags.Static | BindingFlags.NonPublic, null, CallingConventions.Any,
+                new Type[] { typeof(BuildWarningEventArgs) }, null) ?? throw new Exception("Could not find FormatEventMessage(BuildWarningEventArgs).");


Using reflection here concerns me greatly. @rainersigwald, do you have suggestions for a better way to accomplish this?

For context, we're trying to test our parsing of msbuild error message output in Vbc.exe

Sorry, missed this. I agree that this feels icky but I'm afraid I don't have a better idea at the moment.

333fred · 2021-01-08T20:54:36Z

@jaredpar, if you have some time can you take a look at this PR?

jcouv

LGTM (iteration 6). Thanks for figuring out a way to unittest the change.

jcouv · 2021-01-27T04:15:38Z

@jaredpar @RikkiGibson Something strange is happening with CI. The Spanish legs fail after retry, but when I try to get details I get "Build not found" (and no details). Any clues?

RikkiGibson · 2021-01-27T05:36:25Z

The build might be too old to retry. I suggest merging master into the PR branch.

…berrororwarning

adambiser · 2021-01-27T06:00:58Z

The unit tests fail in Spanish because it's checking for the English error message text.

https://github.com/dotnet/roslyn/pull/47833/files#diff-10b671e15f3cd08b75ffd476f257613f8f99459752c107ba458ae68def5c02fc

Assert.Contains("\Test Path (123)\hellovb.vb(6) : warning BC40008: 'Public Property x As Integer' is obsolete.", output.ToString())
Assert.Contains("\Test Path (123)\hellovb.vb(6) : error BC30512: Option Strict On disallows implicit conversions from 'Double' to 'Integer'.", output.ToString())
Assert.Contains("\Test Path (123)\hellovb.vb(7) : error BC30451: 'asdf' is not declared. It may be inaccessible due to its protection level.", output.ToString())

But in Spanish it should check for

\Test Path (123)\hellovb.vb(6) : warning BC40008: 'Public Property x As Integer' está obsoleto.
\Test Path (123)\hellovb.vb(6) : error BC30512: Option Strict On no permite conversiones implícitas de 'Double' a 'Integer'.
\Test Path (123)\hellovb.vb(7) : error BC30451: 'asdf' no está declarado. Puede que sea inaccesible debido a su nivel de protección.

I guess it needs <ConditionalFact(GetType(IsEnglishLocal))> to be skipped for the Spanish checks?

jcouv · 2021-01-27T06:02:49Z

Thanks @adambiser!
@Youssef1313 I'll let you update. Thanks

src/Compilers/Core/MSBuildTask/Vbc.cs

Youssef1313 · 2022-01-18T12:08:28Z

Sorry for the delay here. The last commit should hopefully pass CI.

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb

src/Compilers/Core/MSBuildTask/ManagedCompiler.cs

src/Compilers/Core/MSBuildTask/Vbc.cs

* upstream/main: (1035 commits) Add missing header Mark IVSTypeScriptFormattingServiceImplementation as optional, but require it in the constructor Fix Go To Base for a symbol with a single metadata location (dotnet#58965) [EnC] Store entire spans instead of line deltas (dotnet#58831) Delete CodeAnalysisRules.ruleset (dotnet#58968) Allow xml docs to still be created when 'emit metadata only' is on. (dotnet#57667) Fix ParseVBErrorOrWarning (dotnet#47833) Update parameter nullability to match implementation Ensure CSharpUseParameterNullCheckingDiagnosticAnalyzer works with nullable parameters Add tests for issues fixed by previous PR (dotnet#58764) Update src/Features/CSharp/Portable/Completion/CompletionProviders/ExplicitInterfaceMemberCompletionProvider.CompletionSymbolDisplay.cs Disallow null checking discard parameters (dotnet#58952) Add extension method Escape type arguments Few fixes Update tests. Add Analyzers layer to CODEOWNERS Add formatting analyzer test for param nullchecking (dotnet#58936) Move reading HideAdvancedMembers option up (dotnet#58747) List patterns: Slice value is assumed to be never null (dotnet#57457) ...

Fix ParseVBErrorOrWarning

a884a58

Youssef1313 requested a review from a team as a code owner September 18, 2020 14:50

Youssef1313 commented Sep 18, 2020

View reviewed changes

src/Compilers/Core/MSBuildTask/Vbc.cs Show resolved Hide resolved

Update Vbc.cs

7ca41e3

333fred approved these changes Sep 21, 2020

View reviewed changes

internal -> private

f0fab90

jinujoseph added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Sep 25, 2020

jaredpar reviewed Dec 10, 2020

View reviewed changes

adambiser added 3 commits December 11, 2020 05:32

Added test to verify error reporting from the VB compiler.

4749e5f

Verifies the text used for Microsoft.Build.Tasks.CodeAnalysis.UnitTests.CheckErrorAndWarningParsing.

Added test to verify that column numbers insert correctly.

c108dd0

ErrorLoggerEngine was added to capture the parse results as produced by Microsoft.Build.Shared.EventArgsFormatting.

Needs a space between the end parenthesis and the colon.

f7da91b

Youssef1313 commented Dec 11, 2020

View reviewed changes

333fred reviewed Dec 11, 2020

View reviewed changes

jcouv approved these changes Jan 26, 2021

View reviewed changes

jcouv closed this Jan 26, 2021

jcouv reopened this Jan 26, 2021

jcouv self-assigned this Jan 26, 2021

Merge remote-tracking branch 'dotnet/master' into pr/47833-fix-parsev…

62f411d

…berrororwarning

jcouv marked this pull request as draft January 27, 2021 06:57

Base automatically changed from master to main March 3, 2021 23:52

Youssef1313 added 3 commits January 17, 2022 18:54

Merge branch 'main' into patch-7

e129fab

Update ErrorLoggerEngine.cs

c716288

Update Vbc.cs

ccc6dd6

Youssef1313 commented Jan 17, 2022

View reviewed changes

src/Compilers/Core/MSBuildTask/Vbc.cs Outdated Show resolved Hide resolved

Youssef1313 added 3 commits January 17, 2022 19:35

Update VbcTests.cs

3a7712e

Merge branch 'main' into patch-7

6f27a94

Make internal and fix spanish test failure

68942e8

Youssef1313 marked this pull request as ready for review January 18, 2022 11:52

runfoapp bot mentioned this pull request Jan 18, 2022

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

333fred reviewed Jan 20, 2022

View reviewed changes

src/Compilers/VisualBasic/Test/CommandLine/CommandLineTests.vb Outdated Show resolved Hide resolved

jaredpar approved these changes Jan 20, 2022

View reviewed changes

src/Compilers/Core/MSBuildTask/ManagedCompiler.cs Outdated Show resolved Hide resolved

src/Compilers/Core/MSBuildTask/Vbc.cs Outdated Show resolved Hide resolved

Address feedback

d00bf7d

jaredpar approved these changes Jan 20, 2022

View reviewed changes

jaredpar merged commit 094bee3 into dotnet:main Jan 20, 2022

ghost added this to the Next milestone Jan 20, 2022

Youssef1313 deleted the patch-7 branch January 21, 2022 05:35

RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022

Conversation

Youssef1313 commented Sep 18, 2020

Uh oh!

Uh oh!

adambiser commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 commented Sep 18, 2020

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Sep 24, 2020

Uh oh!

Youssef1313 commented Sep 24, 2020

Uh oh!

jaredpar Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

adambiser commented Dec 10, 2020

Uh oh!

333fred commented Dec 10, 2020

Uh oh!

adambiser commented Dec 10, 2020

Uh oh!

333fred commented Dec 10, 2020

Uh oh!

adambiser commented Dec 10, 2020

Uh oh!

Youssef1313 left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

adambiser Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

333fred Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

333fred Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

rainersigwald Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

333fred commented Jan 8, 2021

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jan 27, 2021

Uh oh!

RikkiGibson commented Jan 27, 2021

Uh oh!

adambiser commented Jan 27, 2021

Uh oh!

jcouv commented Jan 27, 2021

Uh oh!

Uh oh!

Youssef1313 commented Jan 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

adambiser commented Sep 18, 2020 •

edited

Loading