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

test: Use the bash container and a matrix strategy for bash versions #616

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

lens0021
Copy link
Contributor

@lens0021 lens0021 commented Nov 24, 2024

There are some change between versions of bash. (See #592 (comment))
So makes a matrix strategy.

@lens0021

This comment was marked as resolved.

@lens0021

This comment was marked as resolved.

Comment on lines 35 to 47
bash_version:
- "5.3"
- "5.2"
- "5.1"
- "5.0"
- "4.4"
- "4.3"
- "4.2"
- "4.1"
- "4.0"
sed:
- "GNU"
- "BusyBox"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All bash_versions should be ignored(skipped) when BusyBox is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All bash_versions should be ignored(skipped) when BusyBox is used.

Sholudn't. But I will fix this when enabling BusyBox test.

@lens0021 lens0021 marked this pull request as ready for review November 25, 2024 02:35
src/compiler.rs Outdated
Some(command)
if env::var("GITHUB_ACTIONS_BASH_CONTAINER").is_ok() {
let mut command = Command::new("/usr/bin/docker");
command.args(["exec", "--workdir", "/root", "--user", "405", "bash", "bash"]);
Copy link
Contributor Author

@lens0021 lens0021 Nov 25, 2024

Choose a reason for hiding this comment

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

Note: A test requires the test run by a non-root user.

Copy link
Member

Choose a reason for hiding this comment

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

maybe it should detect if running inside docker so it will works always

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've inspected it, but I am sorry that it does not make sense for me. Let me explain this.

The test I mentioned is:

import { is_root } from "std/env"
main {
if not is_root() {
echo "Succeeded"
}
}

and the implementation of is_root() is:

amber/src/std/env.ab

Lines 100 to 107 in cba1671

/// Checks if the script is running with a user with root permission.
pub fun is_root(): Bool {
if trust $ id -u $ == "0" {
return true
}
return false
}

I think the best way to know whether the test is run as root should be just the implement of is_root() function. So If we use the best way, the test will be:

if trust $ id -u $ == "0" and is_root() {
    echo "Succeeded"
}

And it seems it tests nothing, because it tests just redundant conditions.

I think the test should be moved out of an amber script, so the best result should be like:

$ amber is_not_root.ab
I am a not root user!
$ sudo amber is_root.ab
I am a root user!

And it is out of the current test process of stdlibs. I am not sure what should I do, so I will leave --user 405 as it.

@Mte90 Mte90 requested review from hdwalters and Ph0enixKM and removed request for hdwalters November 25, 2024 08:51
@Mte90
Copy link
Member

Mte90 commented Nov 25, 2024

Seems that fix also #422

@Mte90
Copy link
Member

Mte90 commented Nov 25, 2024

it seems also that a test fails for busybox

@lens0021
Copy link
Contributor Author

I've excluded the busybox test now because #617 is not so simple to resolve.

Copy link
Contributor

@hdwalters hdwalters left a comment

Choose a reason for hiding this comment

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

I don't see anything glaring, but just questions for now. Also, bear in mind that I know very little about GitHub or CI workflows; please take opinions on the YAML file with a pinch of salt.

sed:
- GNU
bash_version:
- "5.3-alpha"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually want to support alpha Bash versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, when I asked "Do we have a minimum supported version of Bash?", @Ph0enixKM replied "Minimum supported I think would be 3.0." Should probably include earlier versions here.

Copy link
Contributor Author

@lens0021 lens0021 Nov 26, 2024

Choose a reason for hiding this comment

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

Running tests on alpha version will be helpful, if it is allowed to failure. But, iirc, GitHub Actions does not support allow-failure which exists on Travis or other CI systems. So removing alpha version is a valid option.

Copy link
Contributor Author

@lens0021 lens0021 Nov 26, 2024

Choose a reason for hiding this comment

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

The alpha removed! a6e56e1

Copy link
Contributor Author

@lens0021 lens0021 Nov 26, 2024

Choose a reason for hiding this comment

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

3.1 and 3.0 fail. I've created #621, but if bash array introduced at 3.2, it seems to be hard to support the elder versions. (I cannot easily find which bash version the array is introduced)

Copy link
Contributor Author

@lens0021 lens0021 Nov 26, 2024

Choose a reason for hiding this comment

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

I've removed 3.1 and 3.0 from the tests again because of failures.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that to support those version we should focus on them but having the tests to run for those version when we are not ready is just annoying.

Copy link
Contributor

Choose a reason for hiding this comment

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

Along with a builtin to return the Bash version (suggested in a comment on a different PR) we should also add an annotation specifying the minimum Bash version to support a given function or main block.

.github/workflows/rust.yml Show resolved Hide resolved
src/compiler.rs Outdated Show resolved Hide resolved
src/compiler.rs Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
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.

3 participants