Created New ManagedBatchParser project in .NetStandard #762

NiranjanVirtuosity · 2019-01-03T06:51:13Z

-- Solution Restructured.
-- Created ManagedBatchParser as a independent one.
-- Project referred as Nuget Package.

-- Solution Restructured -- Project referred as Nuget Package

ManagedBatchParser project in .NetStandard

coveralls · 2019-01-04T15:49:21Z

Coverage increased (+1.5%) to 73.692% when pulling 67c48db on NiranjanVirtuosity:master into 7cc2636 on Microsoft:master.

Matteo-T · 2019-01-05T00:46:13Z

@NiranjanVirtuosity: is there a way to see the whole new solution? Could you just share it out so I can take a look? Or share your fork/branch perhaps?

kburtram

Please take a look at my comments. Particularly it looks like the resource strings haven't been split up properly.

kburtram · 2019-01-07T21:57:28Z

src/Microsoft.SqlTools.Hosting/Microsoft.SqlTools.Hosting.csproj

+    <PreserveCompilationContext>true</PreserveCompilationContext>
+    <DebugType>portable</DebugType>
+    <PackageOutputPath>../../bin/nuget</PackageOutputPath>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>


what are these changes regarding packaging for? I assume creating a Nuget package. Are we publishing that now?

Yes, that is the plan.

Do we still need to do this?

kburtram · 2019-01-07T21:59:18Z

src/Microsoft.SqlTools.ManagedBatchParser/Localization/sr.cs

+            }
+        }
+
+


Do you know why the SQL Agent resource are in this file? The resource file should only contain the strings used by this project. Please update the resource files so (1) this project only contains it's resources and (2) the project this code was copied from no longer contains unused resources.

I have optimized the String resource files by removing unused keys in Localization\SR.Strings. Now the Sr.cs files contain only needed strings used by the project.

@NiranjanVirtuosity I can't see where you're removing the batch parser string resources from the ServiceLayer project. Should we do that as well if they are no longer needed there?

@kburtram I have removed the unused strings from both ServiceLayer and ManagedBatchParser projects.

@NiranjanVirtuosity hmm, I still see BatchParser_CommentNotTerminated in the ServiceLayer project in your PR. I'd expect this and other BatchParser string to be removed since they are now duplicated in the new project. Do you know why these strings need to be duplicated in both projects?

Yes I removed those in serviceLayer now.

kburtram · 2019-01-08T21:10:10Z

@NiranjanVirtuosity I see the code coverage number is dropped significantly by this PR. Please resolve this prior to merging. Likely you'll need to add the new assembly to the list of DLLs instrumented for the code coverage runs.

… Microsoft.SqlTools.ManagedBatchParser.LocalizationTests

…ManagedBatchParser.LocalizationTests.csproj

kburtram

It looks like the CI builds are broken and the code coverage number is still way down. Also, please rename the new test project to have a more generic name so it could logically contain all batch parser tests.

kburtram · 2019-01-11T17:31:05Z

test/Microsoft.SqlTools.ManagedBatchParser.LocalizationTests/SrTests.cs

+using Xunit;
+using Microsoft.SqlTools.ServiceLayer.BatchParser.ExecutionEngineCode;
+using ManagedBatchParserSr = Microsoft.SqlTools.ManagedBatchParser.SR;
+namespace Microsoft.SqlTools.ManagedBatchParser.LocalizationTests


Could you please rename this project Microsoft.SqlTools.ManagedBatchParser.UnitTests or something like that so it is a bit more generic? I think having an entire project for a singles localization test is a bit overkill, so if you do need a new test project it should be able to contain all batch parser tests.

As a side note, this test doesn't look particularly useful, and I think it's just for code coverage purposes. If it doesn't impact the code coverage numbers, you might consider removing this test.

kburtram · 2019-01-15T02:45:39Z

nuget.config

@@ -4,6 +4,6 @@
    <clear />


it looks there are binary .nupkg files in this PR. These should be the build output from the projects, so I don't think they should be checked into the Git repository, unless I'm missing something.

Nuget packages which were added in bin folder is a temporary solution to fix CI build issue. Once the Package get published it will be referred from Nuget.org

@NiranjanVirtuosity why are the projects using a binary PackageReference instead of a source ProjectReference? Typically code in the same solution would use a direct source reference.

There's a version conflict between the newly added ManagedBatchParser and these projects when we used project reference, So we decided to refer from NuGet.

kburtram · 2019-01-18T06:46:38Z

src/Microsoft.SqlTools.ManagedBatchParser/Microsoft.SqlTools.ManagedBatchParser.csproj

+  <ItemGroup>
+    <PackageReference Include="Microsoft.SqlServer.SqlManagementObjects" Version="150.18040.0-preview" />
+    <PackageReference Include="System.Data.SqlClient" Version="4.6.0" />
+    <PackageReference Include="Microsoft.SqlTools.Hosting" Version="1.0.0" />


why is this a package reference instead of a project reference?

There's a version conflict between the newly added ManagedBatchParser and these projects when we used project reference, So we decided to refer from NuGet.

Please, explain what the conflict is about...

This is error we get, if we use project reference.
Error: CS0433: The type [''] exists in both 'Microsoft.SqlServer.Smo, Version=15.1.18040.0, Culture=neutral, PublishKeyToken=89845dcd8080cc91' and 'Microsoft.SqlServer.Smo, Version=15.1.16168.50, Culture=neutral, PublishKeyToken=89845dcd8080cc91' [*.csproj]

a binary reference isn't the way to go here. it will need to be constantly updated for one. I'm not sure the error, but it appears there is some type of inconsistent SMO references. Please refer to the other projects that are successfully referencing these projects using ProjectReference in this solution for examples.

kburtram · 2019-01-18T06:50:24Z

@NiranjanVirtuosity based on the "improved" code coverage metric it appears the new project isn't being included in the code coverage runs (i.e., since there aren't any new tests the likely explanation for improved code coverage is less code being instrumented). It would be a good idea to add the new project into the code coverage test runs so that the metric is accurately captured.

…rser.UnitTests Include ManagedBatchParser into Code Coverage calculation Fix the CI issue in .csproj file

kburtram · 2019-01-18T21:22:42Z

@NiranjanVirtuosity please remove the binary Nuget packages from the PR prior to merging to master. They should no longer be needed. Also, do we still need to generate the nupkg files during the build for the hosting project? If so, why?

Matteo-T · 2019-01-21T10:46:24Z

src/Microsoft.SqlTools.ServiceLayer/Microsoft.SqlTools.ServiceLayer.csproj

@@ -34,6 +33,7 @@
  <ItemGroup>
    <ProjectReference Include="../Microsoft.SqlTools.Hosting/Microsoft.SqlTools.Hosting.csproj" />
    <ProjectReference Include="../Microsoft.SqlTools.Credentials/Microsoft.SqlTools.Credentials.csproj" />
+    <ProjectReference Include="..\Microsoft.SqlTools.ManagedBatchParser\Microsoft.SqlTools.ManagedBatchParser.csproj" />


I would appear that the convention is to use forward slashes. Pick one and sick with.

I meant both \ should become /

I replaced those.

…viceLayer.csproj -- <PackageOutputPath> ,<GeneratePackageOnBuild> removed from Hosting and ManagedBatchParser

… ServiceLayer

kburtram · 2019-01-23T23:40:56Z

test/CodeCoverage/codecoverage.bat

@@ -67,6 +71,8 @@ SET SERVICECODECOVERAGE=FALSE

 %CODECOVERAGETOOL% -mergeoutput -register:user -target:dotnet.exe -targetargs:"test %REPOROOT%\test\Microsoft.SqlTools.ServiceLayer.IntegrationTests\Microsoft.SqlTools.ServiceLayer.IntegrationTests.csproj %DOTNETCONFIG%" -oldstyle -filter:"+[Microsoft.SqlTools.*]* +[MicrosoftSqlToolsServiceLayer*]* +[MicrosoftSqlToolsCredentials*]* [Microsoft.SqlTools.*]* -[xunit*]* -[Microsoft.SqlTools.ServiceLayer.Test*]* -[Microsoft.SqlTools.ServiceLayer.*Test*]*  -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Agent.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.DisasterRecovery.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Profiler.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Admin.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Management.*" -output:coverage.xml -hideskipped:All -searchdirs:%REPOROOT%\test\Microsoft.SqlTools.ServiceLayer.IntegrationTests\bin\Debug\netcoreapp2.2

+%CODECOVERAGETOOL% -mergeoutput -register:user -target:dotnet.exe -targetargs:"test %REPOROOT%\test\Microsoft.SqlTools.ManagedBatchParser.UnitTests\Microsoft.SqlTools.ManagedBatchParser.UnitTests.csproj %DOTNETCONFIG%" -oldstyle -filter:"+[Microsoft.SqlTools.*]* +[MicrosoftSqlToolsServiceLayer*]* +[MicrosoftSqlToolsCredentials*]* [Microsoft.SqlTools.*]* -[xunit*]* -[Microsoft.SqlTools.ServiceLayer.Test*]* -[Microsoft.SqlTools.ServiceLayer.*Test*]*  -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Agent.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.DisasterRecovery.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Profiler.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Admin.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Management.*" -output:coverage.xml -hideskipped:All -searchdirs:%REPOROOT%\test\Microsoft.SqlTools.ServiceLayer.IntegrationTests\bin\Debug\netcoreapp2.2


searchdirs path should refer to Microsoft.SqlTools.ManagedBatchParser.UnitTests bin directory rather than IntegrationTests directory.

kburtram · 2019-01-23T23:42:00Z

test/DummyProject_13/UnitTest1.cs

+using System;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace DummyProject_13


why is this DummyProject included in the PR?

-- BatchParser folder moved from ServiceLayer.Integration.Test to ManagedBatchParser.Unit.Tests

kburtram · 2019-01-25T18:52:38Z

test/CodeCoverage/coverage_test.xml

@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="utf-8"?>
+<CoverageSession xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="4.6.684.0">


why is this file being added? i.e., why do we need it checked in?

kburtram · 2019-01-25T18:54:01Z

.gitignore

+*.Designer.cs
+/test/CodeCoverage/codecoverage - Copy.bat


do we need this line in the .gitignore? It doesn't seem like a general file name that will be in most peoples' workspaces.

kburtram · 2019-01-25T18:54:47Z

src/Microsoft.SqlTools.ServiceLayer/Microsoft.SqlTools.ServiceLayer.csproj.BAK

@@ -12,7 +12,7 @@
    <DefineConstants>$(DefineConstants);NETCOREAPP1_0;TRACE</DefineConstants>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <PreserveCompilationContext>true</PreserveCompilationContext>
-    <DebugType>portable</DebugType>
+    <DebugType>full</DebugType>


why are we checking in the .BAK files that get generated during code coverage runs. Please remove all extraneous content from the PR.

@NiranjanVirtuosity: can you address these really old comments? Please, try to be diligent in what you submit. There seems to be quite a bit of junk in your commits...

kburtram · 2019-01-25T18:56:20Z

@NiranjanVirtuosity it looks like the AppVeyor build failed on the latest iteration. I also left some new comments related to various extraneous content in current PR iteration that should be removed prior to merging.

-- testCases included for CodeCoverage in BatchParserTests.cs

-- gitignore igored in check-in -- Microsoft.SqlTools.ServiceLayer.csproj.BAK removed --

…gedBatchParser

1. Microsoft.SqlTools.Hosting 2. Microsoft.SqlTools.ManagedBatchParser

kburtram · 2019-02-08T04:19:52Z

We took this change in #774.

NiranjanVirtuosity and others added 3 commits December 26, 2018 07:59

-- Created New ManagedBatchParser project in .NetStandard

6ffb78a

-- Solution Restructured -- Project referred as Nuget Package

Merge pull request #1 from NiranjanVirtuosity/ManagedBatchParser_Project

9496c2e

ManagedBatchParser project in .NetStandard

Merge remote-tracking branch 'upstream/master'

35e6226

Changed build order and added PackageOutputPath to resolve CI failure

a01c1c8

kburtram requested changes Jan 8, 2019

View reviewed changes

NiranjanVirtuosity added 3 commits January 8, 2019 00:03

Merge remote-tracking branch 'upstream/master'

38d52d1

To resolve CI error

076134f

SR.String file optimised according to project.

40aeb32

NiranjanVirtuosity added 5 commits January 10, 2019 19:45

Removing duplicate String files from ServiceLayer

11032ef

xmlns="http://schemas.microsoft.com/developer/msbuild/2003" added in…

5198a89

… Microsoft.SqlTools.ManagedBatchParser.LocalizationTests

" MSBuild 2003 " is removed in project element at Microsoft.SqlTools.…

d496b83

…ManagedBatchParser.LocalizationTests.csproj

Builded by MSBuild from local

8daaceb

xmlns added

7b3dcec

kburtram requested changes Jan 11, 2019

View reviewed changes

kburtram reviewed Jan 15, 2019

View reviewed changes

kburtram reviewed Jan 18, 2019

View reviewed changes

NiranjanVirtuosity added 2 commits January 18, 2019 15:05

Rename ManagedBatchParser.LocalizationTests project to ManagedBatchPa…

1d2e4c8

…rser.UnitTests Include ManagedBatchParser into Code Coverage calculation Fix the CI issue in .csproj file

Microsoft.SqlTools.Hosting referred back as project instead of package

c2a90f4

ManagedBatchParser referred as Project instead of Nuget in ServiceLayer.

77c3111

Matteo-T reviewed Jan 21, 2019

View reviewed changes

NiranjanVirtuosity added 2 commits January 21, 2019 02:56

-- "..\Microsoft" replace as "../Microsoft" in Microsoft.SqlTools.Ser…

c04742a

…viceLayer.csproj -- <PackageOutputPath> ,<GeneratePackageOnBuild> removed from Hosting and ManagedBatchParser

ManagedBatchParser project reference Slash changed from "\" to "/" in…

67c48db

… ServiceLayer

kburtram reviewed Jan 23, 2019

View reviewed changes

dhruvanmurthy mentioned this pull request Jan 24, 2019

Created New ManagedBatchParser project in .NetStandard #767

Closed

NiranjanVirtuosity added 7 commits January 24, 2019 02:35

Test Commit 01: For verifying CodeCoverage

5a543f5

TestCommit 02: For Code Coverage

fe19ee9

TestCommit 03: Ignore this

180251c

TestCommit: 04 Ignore - To fix CI Build issue

4edfabe

TestCommit : Ignore (Fix CI Issue)

4781d74

TestCommit 05: Ignore this

e8241e6

-- ManagedBatchParser.IntegrationTest Removed

4bcf939

-- BatchParser folder moved from ServiceLayer.Integration.Test to ManagedBatchParser.Unit.Tests

kburtram reviewed Jan 25, 2019

View reviewed changes

NiranjanVirtuosity added 9 commits January 28, 2019 11:26

-- <DebugType> Added in ManagedBatchParser.csproj

adb6a50

-- testCases included for CodeCoverage in BatchParserTests.cs

-- coverage_test.xml removed

d8a563f

-- gitignore igored in check-in -- Microsoft.SqlTools.ServiceLayer.csproj.BAK removed --

Merge remote-tracking branch 'upstream/master'

13e3a9e

-- ClusterEndpoint Class added in ReliableConnectionHelper.cs on Mana…

9f9105b

…gedBatchParser

UnWanted ManagedBatchParser Test project removed.

48f6585

-- removing Nuget package files from bin

0b470fb

1. Microsoft.SqlTools.Hosting 2. Microsoft.SqlTools.ManagedBatchParser

-- VerifyExecuteScript TestCase Removed.

e9d796c

"Pause" removed from CodeCoverage.Bat

f13a1ee

-- revert the changes relevant to CodeCoverage

5c0f95d

dhruvanmurthy mentioned this pull request Jan 31, 2019

Created New ManagedBatchParser project in .NetStandard #772

Closed

kburtram closed this Feb 8, 2019

		@@ -67,6 +71,8 @@ SET SERVICECODECOVERAGE=FALSE

		%CODECOVERAGETOOL% -mergeoutput -register:user -target:dotnet.exe -targetargs:"test %REPOROOT%\test\Microsoft.SqlTools.ServiceLayer.IntegrationTests\Microsoft.SqlTools.ServiceLayer.IntegrationTests.csproj %DOTNETCONFIG%" -oldstyle -filter:"+[Microsoft.SqlTools.] +[MicrosoftSqlToolsServiceLayer] +[MicrosoftSqlToolsCredentials] [Microsoft.SqlTools.] -[xunit] -[Microsoft.SqlTools.ServiceLayer.Test] -[Microsoft.SqlTools.ServiceLayer.Test]* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Agent.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.DisasterRecovery.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Profiler.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Admin.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Management.*" -output:coverage.xml -hideskipped:All -searchdirs:%REPOROOT%\test\Microsoft.SqlTools.ServiceLayer.IntegrationTests\bin\Debug\netcoreapp2.2

		%CODECOVERAGETOOL% -mergeoutput -register:user -target:dotnet.exe -targetargs:"test %REPOROOT%\test\Microsoft.SqlTools.ManagedBatchParser.UnitTests\Microsoft.SqlTools.ManagedBatchParser.UnitTests.csproj %DOTNETCONFIG%" -oldstyle -filter:"+[Microsoft.SqlTools.] +[MicrosoftSqlToolsServiceLayer] +[MicrosoftSqlToolsCredentials] [Microsoft.SqlTools.] -[xunit] -[Microsoft.SqlTools.ServiceLayer.Test] -[Microsoft.SqlTools.ServiceLayer.Test]* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Agent.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.DisasterRecovery.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Profiler.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Admin.* -[MicrosoftSqlToolsServiceLayer]Microsoft.SqlTools.ServiceLayer.Management.*" -output:coverage.xml -hideskipped:All -searchdirs:%REPOROOT%\test\Microsoft.SqlTools.ServiceLayer.IntegrationTests\bin\Debug\netcoreapp2.2

		@@ -0,0 +1,5 @@
		<?xml version="1.0" encoding="utf-8"?>
		<CoverageSession xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="4.6.684.0">

		*.Designer.cs
		/test/CodeCoverage/codecoverage - Copy.bat

Created New ManagedBatchParser project in .NetStandard #762

Created New ManagedBatchParser project in .NetStandard #762

Conversation

NiranjanVirtuosity commented Jan 3, 2019

coveralls commented Jan 4, 2019 • edited Loading

Matteo-T commented Jan 5, 2019

kburtram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiranjanVirtuosity Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kburtram commented Jan 8, 2019

kburtram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kburtram commented Jan 18, 2019

kburtram commented Jan 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kburtram commented Jan 25, 2019

kburtram commented Feb 8, 2019

coveralls commented Jan 4, 2019 •

edited

Loading

NiranjanVirtuosity Jan 9, 2019 •

edited

Loading