Skip to content
This repository has been archived by the owner on Jun 21, 2020. It is now read-only.

Jo 13138 #15

Merged
merged 9 commits into from
May 8, 2017
Merged

Jo 13138 #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4b264c5
Moving forward to a using a flow for the user.
jeffrey-opdam May 5, 2017
52e8ef4
More changes to the flow.
jeffrey-opdam May 6, 2017
ba3981d
Working on the flow.
jeffrey-opdam May 6, 2017
c545667
Finished the flow for connecting to an account.
jeffrey-opdam May 6, 2017
5bd8f91
Fixed current unittests.
jeffrey-opdam May 7, 2017
7d41359
Small cleanup in the Authorize controller.
jeffrey-opdam May 7, 2017
47fa876
Some minor fixes.
jeffrey-opdam May 7, 2017
e1f08d4
- Updated Nuget packages.
jeffrey-opdam May 7, 2017
e5ec4ec
Added guards in ConnectDialog
jeffrey-opdam May 7, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/Common/ProductionCodeRules.ruleset
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
<?xml version="1.0" encoding="utf-8"?>
<RuleSet Name="Production code ruleset" Description="This rule set contains all rules except a few which are not appropriate for production code." ToolsVersion="14.0">
<RuleSet Name="Production code ruleset" Description="This rule set contains all rules except a few which are not appropriate for production code." ToolsVersion="15.0">
<IncludeAll Action="Warning" />
<Rules AnalyzerId="Microsoft.Analyzers.ManagedCodeAnalysis" RuleNamespace="Microsoft.Rules.Managed">
<Rule Id="CA1020" Action="None" />
<Rule Id="CA1305" Action="None" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree that we should suppress CA1305.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, my mistake. Then we need to use 'string result = FormattableString.Invariant($"{name}");" for interpolated strings.

</Rules>
<Rules AnalyzerId="StyleCop.Analyzers" RuleNamespace="StyleCop.Analyzers">
<Rule Id="SA1305" Action="Warning" />
Expand Down
2 changes: 1 addition & 1 deletion src/Team-Services-Bot.AcceptanceTests/CommonSteps.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// Licensed under the MIT License. See License.txt in the project root for license information.
// </copyright>
// <summary>
// Contains the specflow steps to perform an echo.
// Contains the common specflow steps.
// </summary>
// ———————————————————————————————

Expand Down
1 change: 0 additions & 1 deletion src/Team-Services-Bot.AcceptanceTests/EchoSteps.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

namespace Vsar.TSBot.AcceptanceTests
{
using System;
using System.Linq;
using FluentAssertions;
using Microsoft.Bot.Connector.DirectLine;
Expand Down
12 changes: 0 additions & 12 deletions src/Team-Services-Bot.AcceptanceTests/Features/Echo.feature
View file
Open in desktop

This file was deleted.

105 changes: 0 additions & 105 deletions src/Team-Services-Bot.AcceptanceTests/Features/Echo.feature.cs
View file
Open in desktop

This file was deleted.

31 changes: 11 additions & 20 deletions src/Team-Services-Bot.AcceptanceTests/Team-Services-Bot.AcceptanceTests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,23 +75,23 @@
<Reference Include="Gherkin, Version=1.0.0.0, Culture=neutral, PublicKeyToken=86496cfa5b4a5851, processorArchitecture=MSIL">
<HintPath>..\packages\SpecFlow.CustomPlugin.2.1.0\lib\net45\Gherkin.dll</HintPath>
</Reference>
<Reference Include="Microsoft.Bot.Builder, Version=3.5.8.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Bot.Builder.3.5.8.0\lib\net46\Microsoft.Bot.Builder.dll</HintPath>
<Reference Include="Microsoft.Bot.Builder, Version=3.5.9.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Bot.Builder.3.5.9.0\lib\net46\Microsoft.Bot.Builder.dll</HintPath>
</Reference>
<Reference Include="Microsoft.Bot.Builder.Autofac, Version=3.5.8.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Bot.Builder.3.5.8.0\lib\net46\Microsoft.Bot.Builder.Autofac.dll</HintPath>
<Reference Include="Microsoft.Bot.Builder.Autofac, Version=3.5.9.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Bot.Builder.3.5.9.0\lib\net46\Microsoft.Bot.Builder.Autofac.dll</HintPath>
</Reference>
<Reference Include="Microsoft.Bot.Connector, Version=3.5.8.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Bot.Builder.3.5.8.0\lib\net46\Microsoft.Bot.Connector.dll</HintPath>
<Reference Include="Microsoft.Bot.Connector, Version=3.5.9.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Bot.Builder.3.5.9.0\lib\net46\Microsoft.Bot.Connector.dll</HintPath>
</Reference>
<Reference Include="Microsoft.Bot.Connector.DirectLine, Version=3.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Bot.Connector.DirectLine.3.0.0\lib\net45\Microsoft.Bot.Connector.DirectLine.dll</HintPath>
<Reference Include="Microsoft.Bot.Connector.DirectLine, Version=3.0.1.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Bot.Connector.DirectLine.3.0.1\lib\net45\Microsoft.Bot.Connector.DirectLine.dll</HintPath>
</Reference>
<Reference Include="Microsoft.IdentityModel.Protocol.Extensions, Version=1.0.40306.1554, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.IdentityModel.Protocol.Extensions.1.0.4.403061554\lib\net45\Microsoft.IdentityModel.Protocol.Extensions.dll</HintPath>
</Reference>
<Reference Include="Microsoft.Rest.ClientRuntime, Version=2.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35, processorArchitecture=MSIL">
<HintPath>..\packages\Microsoft.Rest.ClientRuntime.2.3.6\lib\net45\Microsoft.Rest.ClientRuntime.dll</HintPath>
<HintPath>..\packages\Microsoft.Rest.ClientRuntime.2.3.7\lib\net452\Microsoft.Rest.ClientRuntime.dll</HintPath>
</Reference>
<Reference Include="Microsoft.VisualStudio.TestPlatform.TestFramework, Version=14.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a, processorArchitecture=MSIL">
<HintPath>..\packages\MSTest.TestFramework.1.1.17\lib\net45\Microsoft.VisualStudio.TestPlatform.TestFramework.dll</HintPath>
Expand All @@ -100,8 +100,8 @@
<HintPath>..\packages\MSTest.TestFramework.1.1.17\lib\net45\Microsoft.VisualStudio.TestPlatform.TestFramework.Extensions.dll</HintPath>
<EmbedInteropTypes>False</EmbedInteropTypes>
</Reference>
<Reference Include="Moq, Version=4.7.9.0, Culture=neutral, PublicKeyToken=69f491c39445e920, processorArchitecture=MSIL">
<HintPath>..\packages\Moq.4.7.9\lib\net45\Moq.dll</HintPath>
<Reference Include="Moq, Version=4.7.10.0, Culture=neutral, PublicKeyToken=69f491c39445e920, processorArchitecture=MSIL">
<HintPath>..\packages\Moq.4.7.10\lib\net45\Moq.dll</HintPath>
</Reference>
<Reference Include="Newtonsoft.Json, Version=10.0.0.0, Culture=neutral, PublicKeyToken=30ad4fe6b2a6aeed, processorArchitecture=MSIL">
<HintPath>..\packages\Newtonsoft.Json.10.0.2\lib\net45\Newtonsoft.Json.dll</HintPath>
Expand Down Expand Up @@ -166,11 +166,6 @@
<DesignTime>True</DesignTime>
<DependentUpon>Controller.feature</DependentUpon>
</Compile>
<Compile Include="Features\Echo.feature.cs">
<AutoGen>True</AutoGen>
<DesignTime>True</DesignTime>
<DependentUpon>Echo.feature</DependentUpon>
</Compile>
<Compile Include="Properties\GlobalSuppressions.cs" />
<Compile Include="Properties\AssemblyInfo.cs" />
</ItemGroup>
Expand All @@ -180,10 +175,6 @@
<Generator>SpecFlowSingleFileGenerator</Generator>
<LastGenOutput>Controller.feature.cs</LastGenOutput>
</None>
<None Include="Features\Echo.feature">
<Generator>SpecFlowSingleFileGenerator</Generator>
<LastGenOutput>Echo.feature.cs</LastGenOutput>
</None>
<None Include="packages.config">
<SubType>Designer</SubType>
</None>
Expand Down
8 changes: 4 additions & 4 deletions src/Team-Services-Bot.AcceptanceTests/packages.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@
<package id="Microsoft.AnalyzerPowerPack" version="1.1.0" targetFramework="net462" />
<package id="Microsoft.AspNet.WebApi.Client" version="5.2.3" targetFramework="net462" />
<package id="Microsoft.AspNet.WebApi.Core" version="5.2.3" targetFramework="net462" />
<package id="Microsoft.Bot.Builder" version="3.5.8.0" targetFramework="net462" />
<package id="Microsoft.Bot.Connector.DirectLine" version="3.0.0" targetFramework="net462" />
<package id="Microsoft.Bot.Builder" version="3.5.9.0" targetFramework="net462" />
<package id="Microsoft.Bot.Connector.DirectLine" version="3.0.1" targetFramework="net462" />
<package id="Microsoft.CodeAnalysis.FxCopAnalyzers" version="1.1.0" targetFramework="net462" />
<package id="Microsoft.IdentityModel.Protocol.Extensions" version="1.0.4.403061554" targetFramework="net462" />
<package id="Microsoft.Rest.ClientRuntime" version="2.3.6" targetFramework="net462" />
<package id="Moq" version="4.7.9" targetFramework="net462" />
<package id="Microsoft.Rest.ClientRuntime" version="2.3.7" targetFramework="net462" />
<package id="Moq" version="4.7.10" targetFramework="net462" />
<package id="MSTest.TestAdapter" version="1.1.17" targetFramework="net462" />
<package id="MSTest.TestFramework" version="1.1.17" targetFramework="net462" />
<package id="Newtonsoft.Json" version="10.0.2" targetFramework="net462" />
Expand Down
32 changes: 18 additions & 14 deletions src/Team-Services-Bot.Api.UnitTests/Dialogs/ConnectDialogTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,16 @@
namespace Vsar.TSBot.UnitTests
{
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using System.Web.Http;
using Autofac;
using Autofac.Integration.WebApi;
using Cards;
using Dialogs;
using Microsoft.ApplicationInsights;
using Microsoft.Bot.Builder.Dialogs;
using Microsoft.Bot.Connector;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Resources;

/// <summary>
/// Contains Test methods for <see cref="ConnectDialog"/>
Expand All @@ -44,18 +42,22 @@ public ConnectDialogTests()
[TestMethod]
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend a TestCategory on every test method. I like to define some constants in a TestCategories class (can be a common file that is linked into the test projects). I know we can't use it in SpecFlow tests but we should use it elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to setup a guidance for the team.

public async Task FirstTimeConnectionTest()
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the test should be a little more descriptive so that if it fails I know from the name more about what has gone wrong. I often use a class called "GivenXYZ", with test methods inside called "WhenSomething_ThenThisThingHappens". I don't know what this test is going to test for from the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, we need to setup guidance for the team.

{
var toBot = this.Fixture.CreateMessage();
toBot.From.Id = Guid.NewGuid().ToString();
toBot.Text = "connect anaccount";
var fromId = Guid.NewGuid().ToString();
var toBot1 = this.Fixture.CreateMessage();
toBot1.From.Id = fromId;
toBot1.Text = "Hi";

var toBot2 = this.Fixture.CreateMessage();
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably one of those "religious" debates, but I dislike var at the best of times. I will accept var where the type is explicit on the right hand side, but I really think it should not be used if the type is not clear by inspection. You can't necessarily program to an interface if you use var as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's get some guidance for the team, and refactor after.

toBot2.From.Id = fromId;
toBot2.Text = "connect anaccount";

const string appId = "AnAppId";
const string authorizeUrl = "https://www.authorizationUrl.com";
const string scope =
"vso.agentpools_manage%20vso.build_execute%20vso.chat_manage%20vso.code_manage%20vso.code_status%20" +
"vso.connected_server%20vso.dashboards_manage%20vso.entitlements%20vso.extension.data_write%20" +
"vso.extension_manage%20vso.gallery_acquire%20vso.gallery_manage%20vso.identity%20vso.loadtest_write%20" +
"vso.notification_manage%20vso.packaging_manage%20vso.profile_write%20vso.project_manage%20" +
"vso.release_manage%20vso.security_manage%20vso.serviceendpoint_manage%20vso.test_write%20vso.work_write";
"vso.agentpools%20vso.build_execute%20vso.chat_write%20vso.code%20vso.connected_server%20" +
"vso.dashboards%20vso.entitlements%20vso.extension%20vso.extension.data%20vso.gallery%20" +
"vso.identity%20vso.loadtest%20vso.notification%20vso.packaging%20vso.project%20" +
"vso.release_execute%20vso.serviceendpoint%20vso.taskgroups%20vso.test%20vso.work";

var builder = this.Fixture.Build();
builder.RegisterType<TelemetryClient>();
Expand All @@ -69,19 +71,21 @@ public async Task FirstTimeConnectionTest()
GlobalConfiguration.Configure(config => config.DependencyResolver = new AutofacWebApiDependencyResolver(container));
var root = new RootDialog();

var toUser = await this.Fixture.GetResponse(container, root, toBot);
// First trigger the welcome message.
await this.Fixture.GetResponse(container, root, toBot1);
var toUser = await this.Fixture.GetResponse(container, root, toBot2);

var attachment = toUser.Attachments.FirstOrDefault();
Assert.IsNotNull(attachment, "Expecting an attachment.");

var card = attachment.Content as SigninCard;
var card = attachment.Content as LogOnCard;
Assert.IsNotNull(card, "Missing signin card.");

var button = card.Buttons.FirstOrDefault();
Assert.IsNotNull(button, "Button is missing");

var expected =
$"https://app.vssps.visualstudio.com/oauth2/authorize?client_id={appId}&response_type=Assertion&state={toBot.ChannelId};{toBot.From.Id}&scope={scope}&redirect_uri={authorizeUrl}/";
$"https://app.vssps.visualstudio.com/oauth2/authorize?client_id={appId}&response_type=Assertion&state={toBot2.ChannelId};{toBot2.From.Id}&scope={scope}&redirect_uri={authorizeUrl}/";
Assert.AreEqual(expected.ToLower(), button.Value.ToString().ToLower(), "OAuth url is invalid.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this could be done as multiple unit tests rather than have a lot of asserts in a single test? Extracting the setup code into methods with intent-revealing names will also make the test much more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be one in the test and one in the api code. We can extract to a common.


Expand Down
36 changes: 0 additions & 36 deletions src/Team-Services-Bot.Api.UnitTests/Dialogs/RootDialogTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,6 @@

namespace Vsar.TSBot.UnitTests
Copy link
Contributor

Choose a reason for hiding this comment

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

Why no tests for rootdialog? It seems like rootdialog has lots of functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed.

{
using System;
using System.Threading.Tasks;
using System.Web.Http;
using Autofac;
using Autofac.Integration.WebApi;
using Dialogs;
using FluentAssertions;
using Microsoft.ApplicationInsights;
using Microsoft.Bot.Builder.Dialogs;
using Microsoft.VisualStudio.TestTools.UnitTesting;

/// <summary>
Expand All @@ -33,32 +24,5 @@ public RootDialogTests()
: base(new DialogFixture())
{
}

/// <summary>
/// Temporary
/// </summary>
/// <returns>Nothing.</returns>
[TestMethod]
public async Task EchoDialog()
{
var toBot = this.Fixture.CreateMessage();
toBot.From.Id = Guid.NewGuid().ToString();
toBot.Text = "Test";

var builder = this.Fixture.Build();
builder.RegisterType<TelemetryClient>();
builder
.RegisterType<EchoDialog>()
.As<IDialog<object>>();

var container = builder.Build();
GlobalConfiguration.Configure(config => config.DependencyResolver = new AutofacWebApiDependencyResolver(container));

var root = new RootDialog();

var toUser = await this.Fixture.GetResponse(container, root, toBot);

toUser.Text.Should().Contain("4").And.Contain("Test");
}
}
}