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 test code for stdlib #127

Merged
merged 9 commits into from
Jun 2, 2024
Merged

Conversation

arapower
Copy link
Contributor

@arapower arapower commented May 29, 2024

Added test code for std functions.

Unaddressed Items

  • input function: Couldn't figure out how to write the code
    • Addressed
  • trim_left function: Does not perform as expected
    • Addressed
  • trim_right function: Does not perform as expected
    • Addressed
  • exit function: Unable to validate changes in exit code.
    • Addressed
  • Comprehensive test coverage: Only covered the happy path for now

I am not familiar with Rust, so please point out any problems.

Closes #117

@b1ek
Copy link
Member

b1ek commented May 29, 2024

  • input function: Couldn't figure out how to write the code

you could write code like print(input()) and feed something into stdin via rust, then check that stdout = stdin. i can't implement this today on my own as my workstation broke, but someone else could

@arapower arapower marked this pull request as draft May 29, 2024 11:28
@arapower arapower marked this pull request as ready for review May 30, 2024 11:56
echo join([\"apple\", \"banana\", \"cherry\"], \", \")
}
";
test_amber!(code, "apple,banana,cherry")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
test_amber!(code, "apple,banana,cherry")
test_amber!(code, "apple, banana, cherry")

Since we join with the comma and space between - why don't we add the space there as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if multiple characters are specified for IFS, it will only be one character when output, so for this case, I'll set it to just ,.

Comment on lines 312 to 322
// TODO: Unable to validate changes in exit code.
#[test]
fn exit() {
let code = "
import * from \"std\"
main {
exit(0)
}
";
test_amber!(code, "")
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can leave it like that cuz it's literally running exit command

@@ -11,6 +11,7 @@ similar-string = "1.4.2"
colored = "2.0.0"
itertools = "0.11.0"
clap = { version = "4.4.18", features = ["derive"] }
tempfile = "3.10.1"
Copy link
Member

Choose a reason for hiding this comment

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

This library seems to be well backed. I think it's okay

@@ -1,7 +1,7 @@
pub fun trim(text) {
return "$\{{nameof text}##*( )}"
return unsafe $echo "{text}" | xargs$
Copy link
Member

Choose a reason for hiding this comment

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

This is not a valid trim function as it basically removes all spaces inside as well.

Screenshot 2024-05-30 at 21 53 21

What's the motivation behind this change in the code?

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 have unified the test_amber macro.
As a result, the test using the trim function in the original test_files/str/trim.ab failed.
Therefore, I copied and pasted the definition of the trim function from src/std/main.ab.

Copy link
Member

Choose a reason for hiding this comment

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

Oh okay then we have to fix the trim function in a separate issue

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

LGTM

@arapower
Copy link
Contributor Author

arapower commented Jun 1, 2024

@b1ek Can you take a look at this?

Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

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

lgtm, but it has a lot of repetitive code. not sure if thats a good idea to push it to master like this


#[test]
fn file_read() {
let temp_dir = tempdir().expect("Failed to create temporary directory");
Copy link
Member

Choose a reason for hiding this comment

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

repetitive code


#[test]
fn file_write() {
let temp_dir = tempdir().expect("Failed to create temporary directory");
Copy link
Member

Choose a reason for hiding this comment

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

repetitive code


#[test]
fn file_append() {
let temp_dir = tempdir().expect("Failed to create temporary directory");
Copy link
Member

Choose a reason for hiding this comment

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

repetitive code


#[test]
fn file_exist() {
let temp_dir = tempdir().expect("Failed to create temporary directory");
Copy link
Member

Choose a reason for hiding this comment

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

repetitive code

@arapower
Copy link
Contributor Author

arapower commented Jun 2, 2024

@b1ek Thank you for your commit.

@arapower arapower requested a review from b1ek June 2, 2024 06:44
Copy link
Member

@b1ek b1ek left a comment

Choose a reason for hiding this comment

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

lgtm

@arapower arapower merged commit 0a78048 into amber-lang:master Jun 2, 2024
1 check passed
@arapower arapower deleted the add-test-stdlib branch June 2, 2024 12:42
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.

🧹 Cover all std functions in tests
3 participants