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

Fix usage of version latest not matching internal elements #298

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

Schultzer
Copy link
Contributor

@Schultzer Schultzer commented Jul 13, 2024

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
Copy link
Collaborator

paulo-ferraz-oliveira commented Jul 13, 2024

👋, @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
Copy link
Collaborator

paulo-ferraz-oliveira commented Jul 13, 2024

⚠️ @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
Copy link
Collaborator

@Schultzer, opened #299, in the meantime, and will re-run the tests 3/4 times to make sure there's no obvious flakiness.

@Schultzer
Copy link
Contributor Author

⚠️ @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
Copy link
Contributor Author

👋, @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
Copy link
Collaborator

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
Copy link
Collaborator

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 paulo-ferraz-oliveira self-requested a review July 14, 2024 11:08
Copy link
Collaborator

@paulo-ferraz-oliveira paulo-ferraz-oliveira left a comment

Choose a reason for hiding this comment

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

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)

@paulo-ferraz-oliveira
Copy link
Collaborator

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 Schultzer changed the title Add integration test for latest Fix latest Jul 17, 2024
@Schultzer
Copy link
Contributor Author

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 paulo-ferraz-oliveira changed the title Fix latest Fix usage of version latest not matching internal elements Jul 17, 2024
@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit a54aa91 into erlef:main Jul 17, 2024
63 checks passed
@paulo-ferraz-oliveira
Copy link
Collaborator

Merged. Thanks.

@paulo-ferraz-oliveira
Copy link
Collaborator

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.

Receiveing 404 when using "latest" - Installing Erlang/OTP 27.0 - built on amd64/ubuntu-22.04
2 participants