Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes how giter8 deals with scripted test #408

Merged
merged 2 commits into from
Aug 30, 2019

Conversation

octonato
Copy link
Contributor

This PR tries to align how tests are executed for giter8 templates and how sbt-scripted is expected to work.

It also makes it possible to use it with Play application using the default Play layout for projects.

This was partially done in sbt/sbt#630, but we need to push this further.

See PR comments for more context.

@@ -123,7 +123,7 @@ object Giter8Plugin extends sbt.AutoPlugin {
} else Nil

// copy test script or generate one
val script = new File(out, "test")
val script = new File(out, ts.getName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a general fix, the test script is always created as "test" while the correct name should be the one specified by ts.

// otherwise, we fallback to test.script
val defaultTestScript =
if (file0.exists && file0.isFile) file0
else dir / "g8" / "test.script"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

While sbt/sbt#630 defines that a scripted test can use either "test" or "test.script", giter8 ignores "test.script" and only cares about "test", "giter8.test" or "g8.test".

Moreover, if a Play project using the default layout is built with giter8, it will always try to use the "test" folder as the script file to run. Which will of course fail.

Basically, the change here is saying, if there is a "test" file we will use it, probably defined as such by the user, otherwise we will use "test.script".

In case of Play templates, that will work just fine because there will be no conflict with "test.script".

For existing users, there is a slightly different behaviour. In case the user is not defining its own script, we will create it as "test.script", instead of "test". In case the user is defining is own "test" file, then we respect its decision and keep using "test" as default.

val files = List(file0,
dir / "g8" / "test.script",
dir / "g8" / "giter8.test",
dir / "g8" / "g8.test",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how "giter8.test" and "g8.test" could possibly be used. If we add such a file, for instace, giter8 will pick it, but it won't work with sbt-scripted because it only accepts "test" or "test.script".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, now I see why "giter8.test" and "g8.test" can work. The user can define the test in such a file, but then when it gets run, the content of the file is copied to out / "test" (as seen here).

In the end, the test script is always copied to a file called "test". The 'fix' I did here make it impossible to use "giter8.test" or "g8.test", because it makes the final script to be named as such which sbt-scripted won't recognize.

@octonato
Copy link
Contributor Author

Sorry for this broken PRs. The changes I made won't work unless we update giter8 to use sbt 1.2.1. I will fix it ASAP.

Copy link
Contributor Author

@octonato octonato left a comment

Choose a reason for hiding this comment

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

fixed the tests and upgraded to sbt 1.2.1 in order to be able to use "test.script"

val script = new File(out, "test")
// the final script should always be called "test.script"
// no matter how it was originally called by user
val script = new File(out, "test.script")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally thought this was a bug, but the final script is always called "test".

Unfortunately that conflicts with Play applications using default Play layout.

val script = new File(out, "test")
// the final script should always be called "test.script"
// no matter how it was originally called by user
val script = new File(out, finalScriptName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally thought this was a bug, but the final script is always called "test". That's what allows users to have scripts called "giter8.test" or "g8.test".

Unfortunately that conflicts with Play applications using default Play layout (see sbt/sbt#630).

Since, sbt 1.2.1, scripted tests recognize "test.script" as a valid script name. We can workaround this situation by forcing it to be "test.script" when using sbt 1.x.

@@ -1 +1 @@
sbt.version=1.1.6
sbt.version=1.2.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to 1.2.1 and adapted builds to explicitly include ScriptedPlugin when needed.

@octonato
Copy link
Contributor Author

We have now two issues with in Travis:

  • build for sbt 0.13.x isn't working because of explicity usage of enablePlugins(ScriptedPlugin)
  • build for sbt 1.2.x failing with unexpected reason. Works on my machine, though.

I will check later how I can fix both.

@dominics
Copy link

dominics commented Nov 28, 2018

@renatocaval Just wanted to say thanks for taking this work on - I'm going to find it very useful.

Was a nasty surprise that sbt-giter8 doesn't work when the output has a test directory.

@octonato
Copy link
Contributor Author

Good that you pinged me here. I was side-tracked and kind of forget this one.

@eed3si9n
Copy link
Member

@renatocaval Would you want to come back to this?

@octonato
Copy link
Contributor Author

@eed3si9n, yes, I will eventually. I need to make time for this though.

I'm just back from vacation, I don't think it will require much work to get it over the line.

I will move it to my TODO list.

@octonato
Copy link
Contributor Author

@eed3si9n, I rebased it and fixed conflicts. Tests are now passing. Ready for a review.

@@ -0,0 +1 @@
enablePlugins(ScriptedPlugin)
Copy link
Member

Choose a reason for hiding this comment

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

Why was this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't remember. Can be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@eed3si9n eed3si9n merged commit 1e55a9f into foundweekends:master Aug 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants