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

Add integration test for latest #298

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Schultzer
Copy link
Contributor

@Schultzer Schultzer commented Jul 13, 2024

Description

Add integration test.

@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.

Comment on lines +439 to +440
} else if (spec0 === 'latest') {
version = versions0[versions0.latest]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This, and the change below, do not seem related to the title of the pull request (talks about adding, not fixing). Why is this required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you take a look at the issue. Then you will see that currently there is a bug causing it to return 404, since we didn’t have any integration test, this was not caught.

Before
Erlang/OTP 27.0

After
Erlang/OTP OTP-27.0.1

The changes ensures that the proper version name is used and are tested.

Please notice the missing OTP-.

Comment on lines +25 to +28
- otp-version: 'latest'
elixir-version: 'latest'
rebar3-version: 'latest'
os: 'ubuntu-latest'
Copy link
Collaborator

@paulo-ferraz-oliveira paulo-ferraz-oliveira Jul 13, 2024

Choose a reason for hiding this comment

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

This would mean that if e.g. OTP, Elixir or rebar3 were to fail in latest the action's results would be affected (which we don't want). As an example, Erlang/OTP Windows 26.2.5.2 has issues installing (I don't know the origin of those), but that shouldn't affect the action's CI results (which I'm pretty sure the results for windows.yml will show they won't because the issue in not in latest, but the previous concern still holds). Having CI fail and discussing fixes upstream should not be the action's concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is your suggestion for an integration test?

@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.

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