Inherent flaw in PrimeFactor exercise

[nathanchere]

I understand the motivation behind implementing a test which exceeds the 32-bit range:

    [Fact]
    public void Factors_include_a_large_prime()
    {
        Assert.Equal(new[] { 11, 9539, 894119 }, PrimeFactors.Factors(93819012551));
    }

but I think it is misguided at best if not encouraging questionable practice to then have the solution template mixing 32- and 64-bit (public static int[] Factors(long number)) and the tests all testing against an int instead of long. If the student is meant to implement support for 64-bit numbers it should also test against 64-bit values.

[nathanchere]

As a more concrete example, try to test this:

 PrimeFactors.Factors(9883692017);

This is not possible to calculate when the return type is defined as int[] instead of long[]. Tests shouldn't impose expectations which require changing the signature of the provided starter code and all other tests to properly support.

[robkeim]

Thanks for reporting this @nathanchere. Currently, our tests are generated based off of a template that is unified across the different language tracks. Could you open an issue to update that template?

The repository with the templates for the different exercises is located here:
https://github.com/exercism/problem-specifications/issues

I think it would make sense to have that last test case added that would force consistent typing between the input and return value.

[ErikSchierboom]

@robkeim We could also update the existing test case to make things consistent with working with long values. What do you think about returning a long[]?

[robkeim]

Yeah we can definitely do that. Were you thinking of doing that instead of updating the canonical data or in additional to updating it?

[nathanchere]

I think it would make sense to have that last test case added that would force consistent typing between the input and return value.

I might have not explained it well enough. It would make no sense to test the consistent typing between input and return value because these are both provided by the starter code. To have a consistent type between these, the student would need to change the main method signature (public static int[] Factors(long number)) and/or all of the tests. This is not something that seems like a reasonable thing to expect of the student (if starter code is provided, I would more so assume you are expected to conform to the structure provided) or that it should be implemented as an additional test.

[ErikSchierboom]

Were you thinking of doing that instead of updating the canonical data or in additional to updating it?

I'm not sure. At the minimum, I'd like to update our tests and stub files to use long values.

[robkeim]

I took a first stab at creating a PR to update the test to use longs, but I ran into an issue. You can find the details in #1310

[robkeim]

This was fixed by @ErikSchierboom in PR #1311

[ErikSchierboom]

@robkeim Actually, I've not fixed this completely in my PR, as I forgot to convert the expected and return value to long. Would you be willing to fix that in this PR?

[robkeim]

Woops... I missed that in the PR review. Yeah I can take care of it, but won't be able to get to it for a couple of days.

[ErikSchierboom]

No problem!