3.0 Code Updates #204

icnocop · 2021-02-21T11:32:05Z

Migrated to SDK-style project
Upgraded to SpecFlow 3
Upgraded to MSTest.Framework and MSTest.Adapter
Added ability to attach files to MSTest test results context
Added ability to set the browser time zone for Microsoft Edge (Chromium) and Google Chrome
Added web driver logging support
Support Win App Driver, keeping track of the page/window/dialog history
Added TempWebElement
Added support for double-clicking, right-clicking, and mouse cursor hover
Added support for closing the browser/dialog
Added support for entering text
Added support for maximizing and switching windows, and focusing the window before clicking on elements
Added support for navigating to the previous page (going back) and refreshing the page
Added support for validating the page URL query string
Added support for setting element data using JavaScript
Added support for setting the browser page load strategy from the configuration
Added support for validating virtual properties
Added support for page sections and contexts
Added support for waiting until validation table
Improved support for elements that require custom logic to get and set values

Fixes #48
Fixes #66
Fixes #71
Fixes #118
Fixes #170
Fixes #171
Fixes #176
Fixes #183
Fixes #185
Fixes #186
Fixes #187
Fixes #192
Fixes #197

Migrated to SDK-style project Upgraded to SpecFlow 3 Upgraded to MSTest.Framework and MSTest.Adapter Added ability to attach files to MSTest test results context Added ability to set the browser time zone for Microsoft Edge (Chromium) and Google Chrome Added web driver logging support Support Win App Driver, keeping track of the page/window/dialog history Added TempWebElement Added support for double-clicking, right-clicking, and mouse cursor hover Added support for closing the browser/dialog Added support for entering text Added support for maximizing and switching windows, and focusing the window before clicking on elements Added support for navigating to the previous page (going back) and refreshing the page Added support for validating the page URL query string Added support for setting element data using JavaScript Added support for setting the browser page load strategy from the configuration Added support for validating virtual properties Added support for page sections and contexts Added support for waiting until validation table Improved support for elements that require custom logic to get and set values

dpiessens · 2021-02-21T14:11:21Z

Hi Rami,
This is a big contribution, thanks! Just as a heads up, the review on this is going to take a while. 247 files and 13 issues in a PR is a lot to mentally digest. I'll make comments as we go, so others can understand the conversation. I do think we should version this as 3.0 given the number of changes.

One larger theme that comes to mind is should we continue support of CodedUI. Based on this article: https://devblogs.microsoft.com/devops/changes-to-coded-ui-test-in-visual-studio-2019/ the framework seems to be deprecated. Thoughts?

.github/workflows/main.yml

icnocop · 2021-02-21T20:33:09Z

It's true that Coded UI is being deprecated.
I'm thinking we can at least try to keep it around as long as it's not too hard to maintain.

I'm not expecting all features in SpecBind to be implemented in Coded UI.
For example, some features may only exist when using Selenium.

dpiessens

Thanks again for all the hard work adding this to the project. There are a decent number of comments in here, a few nits but mostly design based ones. A few high level comments:

I'm still a bit concerned with the size of this PR overall, when I've added actions it makes it easy to check off all the boxes at the step, action, driver, what each platform implements and docs when you can see it. We need at a minimum to change the PR name to '3.0 Code Updates' and then document them all in the description alongside the issue.
Since docs are now embedded in the code base we should have documentation updates
The whole application concept needs to be covered and we should think about wither it needs to be in a separate assembly. Long term I see this running on .NET core and some of the interop code makes that challenging.
Please make sure that any new actions, classes etc. added have unit tests covering them as close to 100% as possible. With few maintainers, high test coverage is the only way we can refactor without the worry that we broke something.
I appreciate the switch to GH actions, I do want to make sure we aren't loosing any steps along the way. I haven't had time to do a full build comparison yet, but I'd like to.

dpiessens · 2021-02-21T14:16:07Z

examples/FullDemo/ContosoUniversity.AcceptanceTests/App.config

@@ -13,9 +13,5 @@
    <stepAssemblies>
      <stepAssembly assembly="SpecBind"/>
    </stepAssemblies>
-    <unitTestProvider name="MsTest"/>
-    <plugins>
-      <add name="SpecBindGeneratorPlugin" type="Generator"/>


What's the new plugin strategy here? does it follow: https://docs.specflow.org/projects/specflow/en/latest/Extend/Plugins.html

From https://docs.specflow.org/projects/specflow/en/latest/Integrations/CodedUI.html#introduction, "Coded UI is no longer supported with SpecFlow 3."

Instead of using a plugin to add attributes to the generated test class, there are explicit calls Playback.Initialize(); and Playback.Cleanup();.

This requires adding SpecBind.CodedUI as a step assembly in the application configuration file.

I'm thinking we can add this to App.config.install.xdt so it's automatically added when installing the SpecBind.CodedUI nuget package in a subsequent PR.

dpiessens · 2021-02-21T14:17:20Z

.gitignore

@@ -115,3 +114,4 @@ UpgradeLog*.XML
 src/SpecBind.sln.DotSettings
 examples/FullDemo/ContosoUniversity/App_Data/School.sdf
 /examples/FullDemo/ContosoUniversity/Properties/PublishProfiles/ContosoUniversity.pubxml
+*.feature.cs


Should feature files really be excluded from SCM?

Only the generated code-behind is being excluded.

They're automatically generated when you build the project anyway.

Otherwise, if the feature file is changed, then you will always see two changed files, when actually you just changed one.

I also started to exclude the generated Code. The Feature Folder looks neat now, so that 'Business' Users have fewer excuses.

dpiessens · 2021-02-21T14:22:03Z

src/SpecBind.CodedUI.IntegrationTests/Features/AlertTests.feature

-	  And I see an alert box and select Ok
-	 Then I see
+@mstest:DeploymentItem:TechTalk.SpecFlow.MSTest.SpecFlowPlugin.dll
+@mstest:DeploymentItem:SpecBind.MsTest.Steps.dll


Adding these for users will be a major breaking change. Similar to above, can we make sure that this is the correct way to implement a plugin like this?

There was an issue filed for this here and I recently created a PR to resolve it here, at least for TechTalk.SpecFlow.MSTest.SpecFlowPlugin.dll.

This is only an issue when deployment is enabled in test/run settings using MSTest.
SpecBind.MsTest.Steps.dll is optional, and only required when you want to take advantage of attaching files to test results when using MSTest.

There may be a way to include SpecBind.MsTest.Steps.dll in test/run settings to avoid having to add this to feature files, and I think that's out of SpecBind's scope.

dpiessens · 2021-02-21T14:29:34Z

src/SpecBind.CodedUI/CodedUIBrowserFactory.cs

+            Playback.PlaybackSettings.SmartMatchOptions = SmartMatchOptions.None; // TopLevelWindow | Control
+            Playback.PlaybackSettings.ThinkTimeMultiplier = 0; // 1
+            Playback.PlaybackSettings.WaitForReadyLevel = WaitForReadyLevel.Disabled; // UIThreadOnly
+            Playback.PlaybackSettings.WaitForReadyTimeout = (int)this.Configuration.PageLoadTimeout.TotalMilliseconds; // 60000


We need to document what changes this will have on users. I suspect changing some of them might cause issues.

dpiessens · 2021-02-21T19:22:24Z

src/SpecBind.Selenium.IntegrationTests/SpecBind.Selenium.IntegrationTests.csproj

-  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' ">
-    <DebugSymbols>true</DebugSymbols>
+    <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
+    <UseWindowsForms>true</UseWindowsForms>


Do we need this?

Yes.

GenerateAssemblyInfo has a default value of true which would otherwise conflict with the existing AssemblyInfo.cs files and cause build failures.

For example,

C:\github\icnocop\specbind\src\SpecBind\obj\Debug\net472\SpecBind.AssemblyInfo.cs(14,12,14,54): error CS0579: Duplicate 'System.Reflection.AssemblyCompanyAttribute' attribute C:\github\icnocop\specbind\src\SpecBind\obj\Debug\net472\SpecBind.AssemblyInfo.cs(15,12,15,60): error CS0579: Duplicate 'System.Reflection.AssemblyConfigurationAttribute' attribute C:\github\icnocop\specbind\src\SpecBind\obj\Debug\net472\SpecBind.AssemblyInfo.cs(16,12,16,58): error CS0579: Duplicate 'System.Reflection.AssemblyFileVersionAttribute' attribute C:\github\icnocop\specbind\src\SpecBind\obj\Debug\net472\SpecBind.AssemblyInfo.cs(18,12,18,54): error CS0579: Duplicate 'System.Reflection.AssemblyProductAttribute' attribute C:\github\icnocop\specbind\src\SpecBind\obj\Debug\net472\SpecBind.AssemblyInfo.cs(19,12,19,52): error CS0579: Duplicate 'System.Reflection.AssemblyTitleAttribute' attribute C:\github\icnocop\specbind\src\SpecBind\obj\Debug\net472\SpecBind.AssemblyInfo.cs(20,12,20,54): error CS0579: Duplicate 'System.Reflection.AssemblyVersionAttribute' attribute

If desired, we can try to delete the existing AssemblyInfo.cs files, and remove this MSBuild property, so the AssemblyInfo.cs files are automatically generated, in a subsequent PR.

src/SpecBind/Helpers/ReflectionHelper.cs

dpiessens · 2021-02-27T02:26:57Z

src/SpecBind/Pages/ElementLocatorAttribute.cs

@@ -65,7 +59,7 @@ public ElementLocatorAttribute()
        /// <value>
        /// The name.
        /// </value>
-        public string Name { get; set; }
+        public virtual string Name { get; set; }


I'm not sure attributes should have virtual properties, why do we need them?

This allows me to create a custom ElementLocatorAttribute and at least override the Name property getter so the value is dynamic.

For example, I have use cases where elements on the UI change based on the user's language, or if the UI is custom branded, or if the user is running a trial version.

With a dynamic ElementLocatorAttribute, I can have one feature file, and one page class which can run through these different scenarios.

Otherwise I would have to use a different feature file (or Gherikin scenario), or more complicated steps, or more complicated page class, or use a different page class for example.

dpiessens · 2021-02-27T02:28:06Z

src/SpecBind/Pages/PageNavigationAttribute.cs

@@ -39,7 +39,7 @@ public PageNavigationAttribute(string url)
        /// <value>
        /// The URL.
        /// </value>
-        public string Url { get; private set; }
+        public virtual string Url { get; private set; }


Same question about virtual props here

This has the same reason as my previous comment for the Name property.

When testing with Windows desktop applications, the Url is used for the window's title.

src/SpecBind/SelectionSteps.cs

Using codecov github workflow action

icnocop · 2021-02-28T07:30:23Z

Thank you for the review.

* I'm still a bit concerned with the size of this PR overall, when I've added actions it makes it easy to check off all the boxes at the step, action, driver, what each platform implements and docs when you can see it. We need at a minimum to change the PR name to '3.0 Code Updates' and then document them all in the description alongside the issue.

I changed the name of the PR to "3.0 Code Updates" and will work on the documentation alongside additional tests.

* Since docs are now embedded in the code base we should have documentation updates

I updated the GitHub pages to use a customized version of the just-the-docs jekyll theme for easier navigation and will will work on adding more documentation.

* The whole application concept needs to be covered and we should think about wither it needs to be in a separate assembly. Long term I see this running on .NET core and some of the interop code makes that challenging.

.NET Core support seems like a feature outside the scope of this PR.
I'm not sure if the changes I've made are the only things that would prevent us from migrating to .NET Core.

Creating a separate assembly which adds support for WinAppDriver also seems like a feature outside the scope of this PR.
SpecBind.Selenium isn't currently designed to support initializing Selenium drivers from external assemblies.
I think this can be addressed in a subsequent PR.

* Please make sure that any new actions, classes etc. added have unit tests covering them as close to 100% as possible. With few maintainers, high test coverage is the only way we can refactor without the worry that we broke something.

I can work on improving code coverage by adding tests for the code that was added and changed, as well as adding documentation for the new features.

The coverage is 81.52% right now. It was previously 84.43%.

* I appreciate the switch to GH actions, I do want to make sure we aren't loosing any steps along the way. I haven't had time to do a full build comparison yet, but I'd like to.

Appveyor's environments don't have Visual Studio Enterprise installed, whereas GitHub does.
This allows us to use the official Coded UI dependencies on the build agent's machine instead of the unofficial Coded UI NuGet package.

The only other thing I noticed is Appveyor.yml sends a notification to gitter.im.
This isn't included in the GitHub workflow and I don't think it's a show stopper.
There hasn't been any activity in https://gitter.im/dpiessens/specbind since July of 2018.
I'm not sure if that space should eventually be moved to https://gitter.im/specbind/specbind, and/or GitHub Discussions enabled for this repo.

dpiessens · 2021-03-23T13:22:50Z

Thanks for the updates to date. To focus the discussion down to a few areas:

I'm not sure a discussion forum is needed at this point, so we can ignore that. I'm fine with the GH actions setup, and I will need to move package release processes over to complete that migration.
Thanks to the updates to themes and docs, given your keen eye to complete documentation in the past I figured that would be an important part of the new actions added.
In terms of code coverage, I'm less concerned about the percentage want to focus more what's covered. The steps, actions and top level driver things are the ones I always tried to get as close to 100% as possible as it's small wire-up features there that are the easiest to break and having those tests give confidence when we need to refactor. The lower level interactions are more difficult and if you're confident they work then we should add an [ExcludeFromCoverage] attribute explaining why. I realize I haven't expressed these goals in writing prior, so I'll be adding a contribution guide for this.
I agree that moving to .NET core is a separate PR. I don't want to add additional work to that goal, by taking dependencies on the registry and native methods that might make that difficult. If you're against splitting out the native method portions due to the work involved, then I want to label the windows interaction as a "beta" feature and subject to breakage as we move to .NET core.

skitscha · 2022-06-07T03:54:46Z

Hi, what is left to do on this pull request? There are some features that were added that I would like to leverage.

icnocop · 2022-06-07T23:49:20Z

I had some difficulty trying to isolate the code paths which require test coverage when developing locally to satisfy bullet point # 3.

The feedback loop was taking a little too long.

Do you have any advice or recommendations on a better approach which can quickly identify the code paths that need test coverage?

Thank you.

dpiessens · 2022-06-13T18:23:00Z

Let's leave it as is, if it's not easy to get coverage then it probably isn't worth the work. I'll give it another look and we should be able to merge.

dpiessens reviewed Feb 21, 2021

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

Bumped up version to 3.0.0

1f46733

Deleted generated code

d0b84bf

dpiessens requested changes Feb 27, 2021

View reviewed changes

icnocop changed the title ~~Upgraded to Visual Studio 2019~~ 3.0 Code Updates Feb 28, 2021

icnocop added 6 commits February 27, 2021 16:41

Removed unused code

2db10c4

Removed "Then I choose (.+)" step to follow AAA pattern

fa8f0a8

Consolidated click action methods

18d2533

Using codecov github workflow action

1c3e9a7

Merge pull request #1 from icnocop/coverage

bed518c

Using codecov github workflow action

Using just-the-docs remote theme for github pages

02062be

icnocop force-pushed the master branch from 797dbdd to 02062be Compare February 28, 2021 06:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3.0 Code Updates #204

3.0 Code Updates #204

icnocop commented Feb 21, 2021 •

edited

Loading

dpiessens commented Feb 21, 2021

icnocop commented Feb 21, 2021

dpiessens left a comment

dpiessens Feb 21, 2021

icnocop Feb 28, 2021

dpiessens Feb 21, 2021

icnocop Feb 28, 2021

SpocWeb Jul 21, 2022

dpiessens Feb 21, 2021

icnocop Feb 28, 2021 •

edited

Loading

dpiessens Feb 21, 2021

dpiessens Feb 21, 2021

icnocop Feb 28, 2021

dpiessens Feb 27, 2021

icnocop Feb 28, 2021

dpiessens Feb 27, 2021

icnocop Feb 28, 2021

icnocop commented Feb 28, 2021

dpiessens commented Mar 23, 2021

skitscha commented Jun 7, 2022

icnocop commented Jun 7, 2022 •

edited

Loading

dpiessens commented Jun 13, 2022

3.0 Code Updates #204

Are you sure you want to change the base?

3.0 Code Updates #204

Conversation

icnocop commented Feb 21, 2021 • edited Loading

dpiessens commented Feb 21, 2021

icnocop commented Feb 21, 2021

dpiessens 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

icnocop Feb 28, 2021 • edited Loading

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

icnocop commented Feb 28, 2021

dpiessens commented Mar 23, 2021

skitscha commented Jun 7, 2022

icnocop commented Jun 7, 2022 • edited Loading

dpiessens commented Jun 13, 2022

icnocop commented Feb 21, 2021 •

edited

Loading

icnocop Feb 28, 2021 •

edited

Loading

icnocop commented Jun 7, 2022 •

edited

Loading