Fake.DotNet.Testing.Expecto #1871

jackfoxy · 2018-04-15T05:29:17Z

Does not support netstandard1.6
netstandard1.6 does not support ProcessStartInfo.EnvironmentVariables, and I don't think the effort at rewrite is justified

matthid · 2018-04-15T09:19:25Z

@jackfoxy Not that I'm specifically interested in netstandard1.6. But why don't we use Fake.Core.Process here? This way some other features will be available for this module as well (for example closing timed-out processes at the end of the fake script)

jackfoxy · 2018-04-15T15:59:16Z

@matthid how exactly do you envision that working? run returns ().
Add a timeout TimeSpan to the ExpectoParams?

jackfoxy · 2018-04-15T16:42:47Z

@matthid if you like this https://github.com/jackfoxy/FAKE/blob/8405a1621ec2d9d5521bc31abcb8dc3b4a7fa51a/src/app/Fake.DotNet.Testing.Expecto/Expecto.fs#L96 better, I will merge into the PR.

Add a timeout TimeSpan to the ExpectoParams?

matthid · 2018-04-15T18:41:10Z

Yes I think it is better. I don't think we need timeout. I just wanted to say that using the Fake.Core.Process module has some advantages over raw framework APIs.

Btw do we need to redirect output of expecto? I think maybe Process.execSimple is enough here?

jackfoxy · 2018-04-15T19:15:56Z

@matthid PR updated
I'm suffering under the slow intellisense resolution of VS, which makes API discovery clunky. I may have to switch to Ionide, like everyone else.

matthid

Looks good thanks! I just broke some of the code with the latest release (sorry). Could you try to get it to green again? (Let me know if you need help)

matthid · 2018-04-15T20:45:11Z

src/app/Fake.DotNet.Testing.Expecto/Expecto.fs

+
+open Fake.Core
+open Fake.Core.Environment
+open Fake.Core.StringBuilder


RequireQualifiedAccess was added to StringBuilder in rc005

matthid · 2018-04-15T20:45:37Z

src/app/Fake.DotNet.Testing.Expecto/Expecto.fs

@@ -0,0 +1,135 @@
+/// Contains tasks to run [Expecto](https://github.com/haf/expecto) unit tests.
+module Fake.DotNet.Testing.Expecto


I think we should add RequireQualifiedAccess here

matthid · 2018-04-15T20:46:46Z

src/app/Fake.DotNet.Testing.Expecto/Expecto.fs

+module Fake.DotNet.Testing.Expecto
+
+open Fake.Core
+open Fake.Core.Environment


RequireQualifiedAccess was added to Environment in rc005

I think more than happened to RequireQualifiedAccess Environment.

I now get this runtime error

Your environment variables look like they are set manually, but you are missing the default variables. Use the `Process.` helpers to change the 'Environment' field to inherit default values! See https://github.com/fsharp/FAKE/issues/1776#issuecomment-365431982

I had to change env variable handling.

This was introduced somewhere in the beta phase. This will prevent incorrect usage of the API. Basically you almost always want to inherit the default environment variables, but you replaced them with your own map -> this error occurs.

matthid · 2018-04-15T20:47:43Z

src/app/Fake.DotNet.Testing.Expecto/Expecto.fs

+                        UseShellExecute = false 
+                        // Pass environment variables to the expecto console process in order to let it detect it's running on TeamCity
+                        // (it checks TEAMCITY_PROJECT_NAME <> null specifically).
+                        Environment = environVars() |> Map } )


Can we just leave it out? I think this is the default.

matthid · 2018-04-15T20:52:44Z

src/app/Fake.DotNet.Testing.Expecto/Expecto.fs

+open System
+
+/// CLI parameters available if you use Tests.runTestsInAssembly defaultConfig argv in your code:
+type ExpectoParams =


As it is part of the Expecto-module we could

either call it Params (if we want to, users would then use Expecto.Params)

move it out of the Expecto module and leave it ExpectoParams

What do you think? We probably even should have some guideline around this and make it more uniform in fake 6

Shortening to Params.

jackfoxy · 2018-04-16T00:19:08Z

PR updated

matthid · 2018-04-17T15:55:04Z

src/app/Fake.DotNet.Testing.Expecto/Expecto.fs

+                        WorkingDirectory = workingDir } 
+                    // Pass environment variables to the expecto console process in order to let it detect it's running on TeamCity
+                    // (it checks TEAMCITY_PROJECT_NAME <> null specifically).
+                    |> Process.setEnvironmentVariables (Environment.environVars()) )


It should be working by just deleting this like. AFAICS you don't add any special environment variables or remove them, correct?

I just copied over this from the legacy code based on the comment. Removed.

jackfoxy · 2018-04-17T20:12:47Z

@matthid perhaps the comment about travis and environment variables is to be taken seriously? Travis build failed.

jackfoxy · 2018-04-17T23:37:50Z

I really could not tell from the raw travis log why it failed.

jackfoxy · 2018-04-19T00:43:53Z

@matthid the best I can determine from the Travis log is it stalled for unknown (to me) reason.

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

matthid · 2018-04-20T16:56:37Z

I'll try to figure out what's wrong

Fake.DotNet.Testing.Expecto

a0913ac

use fake process

8405a16

jackfoxy and others added 2 commits April 15, 2018 11:49

Process.execSimple

4e1d06b

Merge branch 'master' into expecto5

9325320

matthid approved these changes Apr 15, 2018

View reviewed changes

some RequireQualifiedAccess in FAKE

9244db3

matthid reviewed Apr 17, 2018

View reviewed changes

unnecesarry environment config

4be412a

matthid changed the base branch from master to rc_6 April 20, 2018 16:51

matthid merged commit 4f8e42d into fsprojects:rc_6 Apr 20, 2018

jackfoxy deleted the expecto5 branch April 20, 2018 20:13

matthid mentioned this pull request Apr 22, 2018

Release Candidate 6 #1873

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fake.DotNet.Testing.Expecto #1871

Fake.DotNet.Testing.Expecto #1871

jackfoxy commented Apr 15, 2018

matthid commented Apr 15, 2018

jackfoxy commented Apr 15, 2018 •

edited

Loading

jackfoxy commented Apr 15, 2018

matthid commented Apr 15, 2018

jackfoxy commented Apr 15, 2018

matthid left a comment

matthid Apr 15, 2018

matthid Apr 15, 2018

matthid Apr 15, 2018

jackfoxy Apr 15, 2018 •

edited

Loading

matthid Apr 17, 2018

matthid Apr 15, 2018

matthid Apr 15, 2018

jackfoxy Apr 15, 2018

jackfoxy commented Apr 16, 2018

matthid Apr 17, 2018

jackfoxy Apr 17, 2018

jackfoxy commented Apr 17, 2018 •

edited

Loading

jackfoxy commented Apr 17, 2018

jackfoxy commented Apr 19, 2018

matthid commented Apr 20, 2018

		@@ -0,0 +1,135 @@
		/// Contains tasks to run [Expecto](https://github.com/haf/expecto) unit tests.
		module Fake.DotNet.Testing.Expecto

Fake.DotNet.Testing.Expecto #1871

Fake.DotNet.Testing.Expecto #1871

Conversation

jackfoxy commented Apr 15, 2018

matthid commented Apr 15, 2018

jackfoxy commented Apr 15, 2018 • edited Loading

jackfoxy commented Apr 15, 2018

matthid commented Apr 15, 2018

jackfoxy commented Apr 15, 2018

matthid 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

jackfoxy Apr 15, 2018 • 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

jackfoxy commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfoxy commented Apr 17, 2018 • edited Loading

jackfoxy commented Apr 17, 2018

jackfoxy commented Apr 19, 2018

matthid commented Apr 20, 2018

jackfoxy commented Apr 15, 2018 •

edited

Loading

jackfoxy Apr 15, 2018 •

edited

Loading

jackfoxy commented Apr 17, 2018 •

edited

Loading