Fix test run

[paulo-ferraz-oliveira]

I want to do this one separate so I isolate a problem and describe how I fixed it. Whatever comes out of this might affect #9.

[paulo-ferraz-oliveira]

While adding "the rebar3-version feature" (from the now-defunct setup-erlang) to this repo, I noticed npm tests was silently failing: https://github.com/erlef/setup-elixir/runs/1742367663?check_suite_focus=true#step:5:8.

My initial commit, in this pull request (828beb4) shows what I did so the failure was not silent. Very simply, I made sure npm test actually ran tests.

My second commit, in this pull request (132746f) shows the changes I made for the tests to pass, but introduces changes not only to the code but the tests themselves.

Let's first take a closer look at index.js. I moved to otpMatch[1] from otpMatch[0] (where previously we could be comparing over OTP-23.2, we're now comparing over 23.2 alone. Now here's the fun part... if otpMatch is null we go to the second branch of the if (return [gitRef, null]); otherwise we go to the now-fixed branch.

Since I have no previous knowledge of implementation requirements I'm kind of forced to guess the implementor's intention, but wait... versions.get(gitRef) never returns OTP- like versions (and that code - getElixirVersions - was left untouched). This reinforces the idea my fix is OK and now, for some reason, tests are still failing. I revisit those and yeah, kind of figured the input had to use OTP- for two reasons: one, what was explained previously about [0] v. [1]; two, getOtpVersions might return with OTP- or without OTP-, but it surely expects versions like 23.2 to be returned (and not OTP-23.2) - look at its if.

If we also look at https://repo.hex.pm/builds/otp/ubuntu-18.04/builds.txt we might find the only exceptions to OTP- -based versions are:

• maint
• maint-<major>
• master

for which we have no tests, at the moment.

[paulo-ferraz-oliveira]

Does this reasoning/fix make sense?

[paulo-ferraz-oliveira]

If we also look at https://repo.hex.pm/builds/otp/ubuntu-18.04/builds.txt we might find the only exceptions to OTP- -based versions are:

• maint
• maint-<major>
• master

And are these supposed to work? If so, we'd need to change

return [gitRef, null]

to

return [gitRef, otpVersion]

Edit: ... but since that function is (in the current version I'm pull requesting from) only affecting the way we get Elixir for installation, I'm not sure it's required at all.

[paulo-ferraz-oliveira]

If no change is required in this pull request, and it would eventually be accepted as-is, I'd propose to simply close it, since it mirrors something already done in #9. It was just segregated to make it easier to reason on.

[starbelly]

There's a problem in your change, I believe, but I'm perplexed why the integration tests aren't failing. Specifically, as the code was before getElixirVersion would always return [gitRef, otpMatch[0]] or return [gitRef, null] In the case of otpMatch[0] it is correct in that it would be the full string that was matched such as OTP-23.2.2 where your code change seems like it will just return 23.2.2 but the bash script to install OTP and the format of the URLs requires the full string OTP-23.2.2.

The alternative path is return [gitRef, null] which makes no sense to me. We should not be passing around null like this and from looking over the code this case will never succeed, we depend on the version being defined and at a first glance I saw no handling of this case. Either we should be checking this within the caller of getElixirVersion or we should be failing fast right there in getElixirVersion.

I see a lot of room for improvement in all this (not your PR, just the code in general). Example :

 23   const [elixirVersion, otpMajor] = await getElixirVersion(
 24     elixirSpec,
 25     otpVersion
 26   )

Well, that variable name is misnomer. We don't return the otpMajor version, we return OTP-23.2.2 (prior to your changes in this PR).

Also,

 vsn = await getElixirVersion('v1.10.x', 'OTP-23')

^ that makes sense to me, but I'd like to see what happens if you make that change just in the tests without the rest of the code changes.

If only we could write github actions in erlang or elixir 😁

[paulo-ferraz-oliveira]

the bash script to install OTP and the format of the URLs requires the full string OTP-23.2.2.

Look at getOTPVersion. It ends with return version ? OTP-${version} : spec, which means that if a version was found (this is the usual case, I believe) OTP- is prepended to it.

[paulo-ferraz-oliveira]

we depend on the version being defined and at a first glance I saw no handling of this case.

Yes, we "kinda" depend on it. If it's null, otpString (from installElixir) will be just the elixir version, no OTP, which is OK for e.g. v0.15.1 a54d9212b89da61ec0dd4227ed24e59f37738fe5 2016-06-02T12:34:37Z (from builds.txt).

[starbelly]

Yes, we "kinda" depend on it. If it's null, otpString (from installElixir) will be just the elixir version, no OTP, which is OK for e.g. v0.15.1 a54d9212b89da61ec0dd4227ed24e59f37738fe5 2016-06-02T12:34:37Z (from builds.txt).

Yeah, so I'm pretty sure @ericmj would be in favor of only supporting back to elixir 1.0 and OTP 17. If so, we can ditch that. We'll need to refactor to check for a few things in the main entry point (i.e., we should be a bit defensive on the way in, so we don't have to do guess work and string mangling later on). Thoughts?

[paulo-ferraz-oliveira]

I prefer to trim stuff on the way in, too. That way I can easily control the innards. All that adding and removing OTP- and v does not help with getting the job done quicker. 😄

[paulo-ferraz-oliveira]

I'm closing this since I still think my fix is correct, as per what I explain in #9.