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

Retrofit Groovy language track tests as Spock specifications #31

Merged
merged 11 commits into from
Nov 28, 2016
Merged

Retrofit Groovy language track tests as Spock specifications #31

merged 11 commits into from
Nov 28, 2016

Conversation

Dispader
Copy link
Contributor

Hey, everyone.

So— I should type up something more descriptive, but I'd rather start the process of having someone look at this Pull request (because I'm sure that it could use some work).

This Pull is an attempt to retrofit tests as Spock specifications for the Groovy track up through the phone-number exercise, as discussed in this Pull.

Other exercises seem to point to the Exercism.io documentation.

Also, instructions for importing into IntelliJ (or other IDE) seems
better off kept as advanced instructions.  The tests can be run
directly from the command line with edits in any editor (or IDE).

These instructions may not be as complete as we'd like, but I feel
we muddy the waters less by keeping the basic instructions on the
simple side.
- Fix indentation
- Make test cases match canonical
- Implement IllegalArgumentException, per canonical data
@Dispader Dispader changed the title Retrofit spock Retrofit Groovy language track tests as Spock specifications Oct 27, 2016
@kytrinyx
Copy link
Member

Thanks so much @Dispader!

@kimdbarnes do you have time to look at this? (I promise I'm going to look for Groovy track maintainers really, really soon ❤️!)

Copy link

@kimdhendrick kimdhendrick left a comment

Choose a reason for hiding this comment

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

@ignore is misspelled in this sentence: "+After the first test(s) pass, continue by commenting out or removing the @Ingore annotations prepending other tests."

@kimdhendrick
Copy link

kimdhendrick commented Oct 30, 2016

Except for the above misspelling, it looks good to me!

@mattphillips
Copy link

I see Gradle was briefly touched upon in #26 I'm just wondering why it's not being used over @Grab to manage dependencies like Spock. I think that understanding a build/ dependency management tool for a language is a vital part of learning the ecosystem?

@Grab('org.spockframework:spock-core:1.0-groovy-2.4')
import spock.lang.*

class SquaresSpec extends Specification {

Choose a reason for hiding this comment

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

These test are a good chance to show off Spock's parameterised tests with something like this for each one:

@Unroll
def 'can square the sum of the numbers up to the given number: #num'() {
    expect:
        new Squares(num).squareOfSums() == expected

    where:
        num   || expected
        5     || 225
        10    || 3025
        100   || 25502500
}

@dvberkel
Copy link

dvberkel commented Oct 31, 2016

I think this is a accurate implementation of the various test using Spock.

I would give my 👍

class HelloWorldSpec extends Specification {

def 'outputs "Hello, World!"'() {
expect: new HelloWorld().hello() == 'Hello, World!'

Choose a reason for hiding this comment

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

I would have the expect and other tags on separate lines.
And I would extract the class instantiation to a field.

Copy link
Contributor Author

@Dispader Dispader Nov 1, 2016

Choose a reason for hiding this comment

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

@gijsleussink I was also wondering about this one— you think that we should introduce helloWorld as a test case variable?

I could get behind that, but I was almost considering making the call static, since our calls just output text, and don't modify any object state, which would make the calls HelloWorld.hello().

If we went as far as a static get method, the call could look like HelloWorld.hello.

Thoughts?

Choose a reason for hiding this comment

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

Yeah, in this case it simplifies. I read the Hello-World exercise and you mainly focus on getting the concept of TDD across. So why not keep the Spec as simple and easily readable as possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gijsleussink What d'you think, sir?

@Grab('org.spockframework:spock-core:1.0-groovy-2.4')
import spock.lang.*

class HammingSpec extends Specification {

Choose a reason for hiding this comment

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

Perhaps it's mentioned somewhere else (in the exercise?), but if I only look at this spec I wouldn't know (the overview of) what a Hamming-distance is and what the letters stand for

Choose a reason for hiding this comment

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

An exercise is assembled from various sources. For example, the description for The Hamming exercise can be found at x-common/exercises/hamming

@tlberglund
Copy link

@mattphillips I'm always 👍 on Gradle, but to my mind @Grab never gets enough love either. In this case it helps the code be more self-contained and removes environment setup tasks. I'm new to Exercism and may conceivably be missing the point, but I think we just want Groovy-based solutions to the standard Exercism programming problems. @Grab seems to me like the move in that case. 😀

@mattphillips
Copy link

@tlberglund I'm not a Groovy dev but @Grab seems to be a bit magical (I'm not sure why you would grab the same dependencies multiple times in different files), although it does fit this use case quite well 😃. However I'd say that learning a new language is not only about the core constructs and syntax but actually learning the ecosystem, which includes build/ dependency management tools like Gradle and how to write tests.

@@ -7,6 +7,6 @@ Run the tests by executing the test script.
$ groovy ./HelloWorldSpec.groovy
```

After the first test(s) pass, continue by commenting out or removing the `@Ingore` annotations prepending other tests.
After the first test(s) pass, continue by commenting out or removing the `@Ignore` annotations prepending other tests.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kimdbarnes !

@tlberglund
Copy link

Oh, @Grab is dark magick alright. But I think the structure of the overall Exercism project is that you solve the same programming problems in different languages. I don't think it's a general platform and language introduction. So I completely agree that Groovy is best built with Gradle and a good introduction to one entails a good introduction to the other, but I'd stand behind the use of @Grab here given the project scope. But I stress that this is merely my $2E-2. :)

@kytrinyx
Copy link
Member

I'd say that learning a new language is not only about the core constructs and syntax but actually learning the ecosystem, which includes build/ dependency management tools like Gradle and how to write tests.

@mattphillips in general I'd agree, however on Exercism we're explicitly targeting a high level of fluency at only the language syntax / standard library / idioms / conventions level, not the larger platform / tool / real-world-codebase level.

@kytrinyx kytrinyx merged commit 8fc4937 into exercism:master Nov 28, 2016
@kytrinyx
Copy link
Member

Thanks everyone, for the discussion!

@Dispader Dispader deleted the retrofit_spock branch February 23, 2017 05:27
@Dispader Dispader mentioned this pull request Oct 11, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants