WIP fix for shell script executable bit and scaffolding for template tests #1990

baronfel · 2018-06-10T15:44:56Z

No description provided.

baronfel · 2018-06-10T16:43:36Z

GAH! We can't actually set the executable bit because of a few outstanding templating/coreclr bugs: dotnet/templating#1547

However we should still add this because a) it'll be fixed at some point, and b) even if it fails it leaves a helpful error message:

The files /private/var/folders/80/y0q8bqgx29bd8g3h0tlt00k00000gn/T/mrisxb4t.jrn/chmod, /private/var/folders/80/y0q8bqgx29bd8g3h0tlt00k00000gn/T/mrisxb4t.jrn/+x, and /private/var/folders/80/y0q8bqgx29bd8g3h0tlt00k00000gn/T/mrisxb4t.jrn/*.sh do not exist.
Unable to apply permissions +x to "*.sh".
Post action failed.
Description: Make scripts executable
Manual instructions: Run 'chmod +x *.sh'

matthid · 2018-06-10T16:49:41Z

@baronfel I don't think we can expect them to change the behavior again, the flag just was used incorrectly all the years - I agree that using it like this is not portable at all. Therefore we should probably change to /bin/sh -c or /usr/bin/env sh -c

baronfel · 2018-06-10T16:51:16Z

that's fair 👍

baronfel · 2018-06-10T16:55:56Z

hard to test the run an executable action in an integration test though because the promtps require user input:

Template is configured to run the following action:
Description: Make scripts executable
Manual instructions: Run 'chmod +x *.sh'
Actual command: /bin/sh -c "echo pwd && chmod +x *.sh"
Do you want to run this action (Y|N)?

baronfel · 2018-06-10T17:09:25Z

Ok, I've added a test for both the tool and project variants. It would be easy to write more. The hard part will be interacting with the tests because now we have to answer 'yes' for each test of the template. For this reason I've marked them pending but I think we should use them when working on the template locally for sure, at least until the input question is solved.

matthid · 2018-06-10T17:11:30Z

We can either try to set all options beforehand via command line or just redirect and write standard input

matthid · 2018-06-10T17:14:53Z

Apparently there is --allow-scripts yes, see dotnet/templating#765 (comment)
Seems to be undocumented for now as I cannot see it in --help

matthid · 2018-06-10T17:19:17Z

src/test/Fake.Dotnet.Cli.IntegrationTests/TemplateTests.fs

@@ -1,11 +1,75 @@
-module Fake.Dotnet.Cli.IntegrationTests.TemplateTests
+module Fake.DotNet.Cli.IntegrationTests.TemplateTests


Please also rename the folder in git, otherwise there will be problems (which is a bit unfortunate to do on windows as you have to temporarily rename to something else)

baronfel · 2018-06-10T17:39:32Z

Ok, I did it via redirecting stdin

matthid · 2018-06-10T19:33:06Z

src/test/Fake.DotNet.Cli.IntegrationTests/TemplateTests.fs

+            x.WithWorkingDirectory(dir)
+             .WithFileName(fullScriptPath)
+             .WithArguments "--help" ) (System.TimeSpan.FromSeconds 60.)
+    |> shouldSucceed "should invoke the script file"


I'd prefer a test which actually checks if ./fake build works (ie all dependencies still work)
Also can we add a failing test (ie that the scripts correctly return nonzero exit-codes on failure). For example by executing ./fake.sh run nonexisting.fsx.

Added a test around ./fake.sh build -t All and ./fake.sh build -t Nonexistent

matthid · 2018-06-10T19:33:13Z

src/test/Fake.DotNet.Cli.IntegrationTests/TemplateTests.fs

+        )
+
+    proc.Start () |> ignore
+    do! Async.Sleep 2000


Do we really need that?

Yeah, I was guessing at how long to wait after invoking the template gen, so that the prompt would be active at the point that I sent the Y character.

Did you try --allow-scripts yes (not the most important - but I guess it would not introduce a potential race condition)

No, I'll give that a try when I add the new tests

worked like a charm.

matthid · 2018-06-10T19:34:19Z

Thanks a lot for taking care of this! Just two more things I noticed.

baronfel · 2018-06-10T20:26:42Z

Alright, if this is green I think we're in a better place.

baronfel · 2018-06-11T16:31:07Z

Is there anything else here I missed that you'd like me to cover?

matthid · 2018-06-11T19:38:27Z

Only the one comment, but otherwise looks good

baronfel · 2018-06-11T19:40:38Z

I'm sorry, which comment do you mean? I thought I just needed to look into --allow-scripts (which we're now using successfully) and adding some more tests around the positive/negative template-script uses (which I added).

matthid

Oh I forgot to actually submit the review, sorry ;)

matthid · 2018-06-11T06:03:30Z

src/test/Fake.DotNet.Cli.IntegrationTests/TemplateTests.fs

+                Directory.ensure tempDir
+                runTemplate tempDir Project
+                let result = invokeScript tempDir scriptFile "build -t Nonexistent" 
+                Expect.isFalse (result.OK && missingTarget "Nonexistent" result) "Should fail to build a target that doesn't exist"


I think because result.OK is false missingTarget will not be executed.
What we want is probably 'isTrue(not OK && missingTarget)'?

ah you are of course correct. I updated the test checks to be more clear about the two conditions.

…ing target

matthid

matthid · 2018-06-11T21:23:51Z

Oh we do not actually run the new tests in the build process, do we?

baronfel · 2018-06-11T21:34:52Z

ah yeah. I'm looking at how to place them post-dotnet-pack right now.

matthid · 2018-06-11T21:41:15Z

You can also integrate it into the DotNetCoreIntegrationTests just call dotnet run like the others

baronfel · 2018-06-11T21:51:07Z

Yeah but I'm relying on the pack task having packaged the nuget package, so I can just install it. That seems harder to orchestrate than just running the template tests sometime after packaging. What do you think?

matthid · 2018-06-11T21:52:14Z

Ah yeah whatever you think is easier :)

baronfel · 2018-06-11T22:05:34Z

Ok, I think I got them. The template tests are a little fragile (meaning towards future changes) because they point to the nuget directory precisely, but they do actually get invoked now.

matthid · 2018-06-11T23:10:25Z

Thanks a lot for taking care of this thing!

rmunn · 2018-06-14T08:04:31Z

dotnet/templating#1547 has been fixed by dotnet/templating#1555, so I suspect it'll be in the next .Net Core bugfix release. But since version 2.1.300 was released with the bug in it, it'll be a while before it's safe to use the chmod post-action (the one with GUID cb9a6cf3-4f5c-4860-b9d2-03a574959774) and trust that it'll work on everyone's system.

fix for shell script executable bit and scaffolding for tests

2ddaca7

write template tests

dbdbbff

matthid reviewed Jun 10, 2018

View reviewed changes

make the tests autonomous

5e22412

rname to fix casing

b02b7f1

matthid reviewed Jun 10, 2018

View reviewed changes

matthid reviewed Jun 11, 2018

View reviewed changes

add a test around building a target, and one around failing on a miss…

e02f56c

…ing target

baronfel force-pushed the executable_scripts branch from 5b611ad to e02f56c Compare June 11, 2018 20:39

matthid approved these changes Jun 11, 2018

View reviewed changes

pull the template tests into the build pipeline

db9327e

matthid merged commit 8064d42 into fsprojects:release/next Jun 11, 2018

baronfel deleted the executable_scripts branch June 19, 2018 16:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP fix for shell script executable bit and scaffolding for template tests #1990

WIP fix for shell script executable bit and scaffolding for template tests #1990

baronfel commented Jun 10, 2018

baronfel commented Jun 10, 2018

matthid commented Jun 10, 2018

baronfel commented Jun 10, 2018

baronfel commented Jun 10, 2018

baronfel commented Jun 10, 2018

matthid commented Jun 10, 2018

matthid commented Jun 10, 2018

matthid Jun 10, 2018

baronfel commented Jun 10, 2018

matthid Jun 10, 2018

baronfel Jun 10, 2018

baronfel Jun 10, 2018

matthid Jun 10, 2018

baronfel Jun 10, 2018

matthid Jun 10, 2018

baronfel Jun 10, 2018

baronfel Jun 10, 2018

matthid commented Jun 10, 2018

baronfel commented Jun 10, 2018

baronfel commented Jun 11, 2018

matthid commented Jun 11, 2018

baronfel commented Jun 11, 2018

matthid left a comment

matthid Jun 11, 2018

baronfel Jun 11, 2018

matthid left a comment

matthid commented Jun 11, 2018

baronfel commented Jun 11, 2018

matthid commented Jun 11, 2018

baronfel commented Jun 11, 2018

matthid commented Jun 11, 2018

baronfel commented Jun 11, 2018 •

edited

Loading

matthid commented Jun 11, 2018

rmunn commented Jun 14, 2018

		@@ -1,11 +1,75 @@
		module Fake.Dotnet.Cli.IntegrationTests.TemplateTests
		module Fake.DotNet.Cli.IntegrationTests.TemplateTests

WIP fix for shell script executable bit and scaffolding for template tests #1990

WIP fix for shell script executable bit and scaffolding for template tests #1990

Conversation

baronfel commented Jun 10, 2018

baronfel commented Jun 10, 2018

matthid commented Jun 10, 2018

baronfel commented Jun 10, 2018

baronfel commented Jun 10, 2018

baronfel commented Jun 10, 2018

matthid commented Jun 10, 2018

matthid commented Jun 10, 2018

Choose a reason for hiding this comment

baronfel commented Jun 10, 2018

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

matthid commented Jun 10, 2018

baronfel commented Jun 10, 2018

baronfel commented Jun 11, 2018

matthid commented Jun 11, 2018

baronfel commented Jun 11, 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

matthid left a comment

Choose a reason for hiding this comment

matthid commented Jun 11, 2018

baronfel commented Jun 11, 2018

matthid commented Jun 11, 2018

baronfel commented Jun 11, 2018

matthid commented Jun 11, 2018

baronfel commented Jun 11, 2018 • edited Loading

matthid commented Jun 11, 2018

rmunn commented Jun 14, 2018

baronfel commented Jun 11, 2018 •

edited

Loading