Commented tests in TwoferTest Class and possibly one case missing

[ilya-khadykin]

While doing two-fer I've noticed that two positive test cases were commented (not sure why).

    @Ignore("Remove to run test")
    @Test
    public void aNameGiven() {
        String input = "Alice";
        String expected = "One for Alice, one for me.";

        assertEquals(expected, twofer.twofer(input));
    }

    @Ignore("Remove to run test")
    @Test
    public void anotherNameGiven() {
        String input = "Bob";
        String expected = "One for Bob, one for me.";

        assertEquals(expected, twofer.twofer(input));
    }

I guess they shouldn't be ignored by default.

Moreover, I suggest adding additional one (but not sure if it complies with task description):

    @Test
    public void emptyStringGiven() {
        String input = "";
        String expected = "One for you, one for me.";

        assertEquals(expected, twofer.twofer(input) );
    }

[Smarticles101]

The readme for the exercise describes the usage of the @Ignore annotations. They're basically there so that people can work on one test case at a time.

The additional test case you suggested isn't in the canonical data for the exercise but it seems like a good idea to me. Thoughts @exercism/java ?

[stkent]

Interesting, as currently written this depends on how we interpret 'no name':

If no name is given, the result should be "One for you, one for me."

Is a blank string 'no name', or is null the only way to represent 'no name'? 🤔

Could be worth asking in problem-specifications, though i'd be surprised if we get a clear consensus on the above :)

[Smarticles101]

The old hello-world exercise used null and/or a blank string to represent no name. The fact that this is different in two-fer suggests that this may have been changed on purpose. Might require some digging to find out.

Update: Didn't take much digging. From exercism/problem-specifications#290 I found that a common opinion seemed to be that null should mean no name given and an empty string should be treated as a valid name

[ilya-khadykin]

@Smarticles101, thanks for clarification!

The issue can be closed then.

But in the real life you are expected to deal with empty Strings too 😷

[stkent]

Slightly more specific reference:

Also agreed that if the language can differentiate between empty string and nil, it should probably treat the empty string as a name.

[stkent]

Might still be worth adding a blank string test to the Java track even though it's not captured in the canonical data (since it wouldn't make sense for all languages in the canonical data).

[ilya-khadykin]

It's your call, I can create PR if you want and mark this test as with @Ignore("Remove to run test")

[stkent]

I think that'd be a good addition, so yes, please! I'd add it as the last test so it's separated from the 'canonical' tests.

[ilya-khadykin]

Sure, will do later today when I get back home

[ilya-khadykin]

@stkent, PR #733 has been created, please review 👨‍💻