search

erlef / setup-beam

Set up your BEAM-based GitHub Actions workflow (Erlang, Elixir, Gleam, ...)
MIT License
378 stars 51 forks source link

Fix usage of version `latest` not matching internal elements #298

Closed Schultzer closed 3 months ago

Schultzer commented 3 months ago

Description

~Add integration test.~ Fix latest

@paulo-ferraz-oliveira, there are some flaky tests or a bug in the version resolution, which is unrelated to my changes.

Closes #297

paulo-ferraz-oliveira commented 3 months ago

👋, @Schultzer.

What tests are flaky in particular? (edit: maybe it was the word "flaky" that got me thinking, since it should mean that upon a re-run there's a possibility the flaky test passes; this would happen with the Windows installer issue, for example - if Erlang/OTP was resolved and e.g. 26.2.5.3 was released for Windows -, but not the 27.0 assumption)

I know that e.g. the installation on Windows, for 26.2.5.2 fails (I detected it in another repo.) and it seems that we're trying to have 27.0 resolved instead of 27.0.1, which is something recent.

paulo-ferraz-oliveira commented 3 months ago

⚠️ @Schultzer, I don't think we'll accept the tests on latest based on what was exposed before: one dependency fails, the action's tests shouldn't be affected.

On the other hand, if your changes to the code are required we'll still consider them, given that the PR title is adjusted.

I'll briefly push to remove tests on Windows 26.2 and to fix the 27.0 comparison (since it was not being properly restricted).

paulo-ferraz-oliveira commented 3 months ago

@Schultzer, opened https://github.com/erlef/setup-beam/pull/299, in the meantime, and will re-run the tests 3/4 times to make sure there's no obvious flakiness.

Schultzer commented 3 months ago

⚠️ @Schultzer, I don't think we'll accept the tests on latest based on what was exposed before: one dependency fails, the action's tests shouldn't be affected.

On the other hand, if your changes to the code are required we'll still consider them, given that the PR title is adjusted.

I'll briefly push to remove tests on Windows 26.2 and to fix the 27.0 comparison (since it was not being properly restricted).

Maybe it would be beneficial to move the latest integration test into it’s own action, which could run nightly, and provide automated status for when the latest are working or not.

Schultzer commented 3 months ago

👋, @Schultzer.

What tests are flaky in particular? (edit: maybe it was the word "flaky" that got me thinking, since it should mean that upon a re-run there's a possibility the flaky test passes; this would happen with the Windows installer issue, for example - if Erlang/OTP was resolved and e.g. 26.2.5.3 was released for Windows -, but not the 27.0 assumption)

I know that e.g. the installation on Windows, for 26.2.5.2 fails (I detected it in another repo.) and it seems that we're trying to have 27.0 resolved instead of 27.0.1, which is something recent.

Well, I would consider the 27.0 test flaky, if in the future it could match on 27.0.1. Which looks like what has happen.

I have no idea why 26.2.5.2 is failing.

paulo-ferraz-oliveira commented 3 months ago

Maybe it would be beneficial to move the latest integration test into it’s own action, which could run nightly, and provide automated status for when the latest are working or not.

Maybe, but that's not something we'll maintain. The purpose of that would eventually be to find issues with the latest versions' installers, or similar, not the action itself, which isn't the purpose of the action. An issue is opened next to erlang/otp, already, and that's most surely where the fix will happen: it's up to developers using latest to take that risk, not the action's.

paulo-ferraz-oliveira commented 3 months ago

Well, I would consider the 27.0 test flaky, if in the future it could match on 27.0.1.

Maybe it's semantics, but a flaky test is one that, upon running, will result in unpredictable results (either it passes now, or it passes later). That isn't the case with the test case I fixed, since it wouldn't ever pass, in the future, once 27.0.x was released.

paulo-ferraz-oliveira commented 3 months ago

With #299 merged, this pull request can be rebased and adapted to the requested code changes. There should be no tests with issues after the rebase.

Schultzer commented 3 months ago

The following changes are requested:

  • remove "integration tests" with online elements (i.e. reverse the changes made to ubuntu.yml and windows.yml)
  • make sure that getVersionFromSpec is well tested for latest as the changed code should've provoked changes in the tests (or there's, otherwise, something missing I don't understand)

Suppose the integration test passes after the code changes. In that case, there is clearly not an issue in getVersionFromSpec but rather between the layers, which is what integration tests are helpful for.

We can conclude that the issue is in getOTPVersions. Or rather the contract between them.

Only an integration test would have caught this. Regardless I have made the requested changes.

paulo-ferraz-oliveira commented 3 months ago

Merged. Thanks.

paulo-ferraz-oliveira commented 3 months ago

This is part of https://github.com/erlef/setup-beam/releases/tag/v1.18.1.