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 basic unit tests #240

Closed
wants to merge 3 commits into from

Conversation

FedericoCeratto
Copy link
Contributor

This change adds basic unit tests and a test harness that compiles code and compare its with an expected value.
If any difference is found the test fail and show a friendly diff and the generated code.

src/tests/mod.rs Outdated Show resolved Hide resolved
src/tests/mod.rs Outdated Show resolved Hide resolved
src/tests/mod.rs Show resolved Hide resolved
src/tests/unit.rs Outdated Show resolved Hide resolved
src/tests/unit.rs Outdated Show resolved Hide resolved
@Ph0enixKM
Copy link
Member

Isn't this accomplishing the same job as the end to end tests we had before?

@FedericoCeratto
Copy link
Contributor Author

@Ph0enixKM these are unit tests (instead of integration tests). The purpose is to (quickly) test the exact generated output.

@Ph0enixKM
Copy link
Member

@FedericoCeratto I know what these do but what's the practical difference? Oh... instead of testing result we test the resulting bash code. But then we'd have to modify a lot of tests since we'll be now working on making the bash look more readable

@FedericoCeratto
Copy link
Contributor Author

@Ph0enixKM Yes, the tests are meant to capture the generated shellscript and reflect improvements in readability. More importantly they would detect any unexpected change in the script layout. The amount of tests should remain very modest.

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.

Looking good! I've got some minor suggestions

fn basic() {
comp(
r#"
import * from "std"
Copy link
Member

Choose a reason for hiding this comment

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

This import is unnecessary

Suggested change
import * from "std"


/// Compile code and compare it with the expected value.
/// If any difference is found panic and show a friendly diff
pub fn comp(source: &str, expected: &str) {
Copy link
Member

Choose a reason for hiding this comment

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

Since comp can be read as compute or compare, can we change this name to a more verbose one?

This is just and example:

Suggested change
pub fn comp(source: &str, expected: &str) {
pub fn compare_bash_output(source: &str, expected: &str) {

Copy link
Member

Choose a reason for hiding this comment

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

maybe more like

Suggested change
pub fn comp(source: &str, expected: &str) {
pub fn compare_strings(source: &str, expected: &str) {

Copy link
Member

Choose a reason for hiding this comment

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

maybe more like

Sounds good too

Comment on lines +30 to +34
// Dedent code blocks based on the first line after `r#"`
let mut lines = expected.lines();
lines.next();
let line = lines.next().unwrap_or("");
let indent = line.chars().take_while(|&c| c.is_whitespace()).count();
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to a separate function?

Comment on lines +54 to +79
let compiled = compiled[FILE_HEADER.len()..]
.trim_end_matches("\n")
.trim_start_matches("\n");

if compiled == exp {
return;
};

let changeset = Changeset::new(&compiled, &exp, " ");

let mut out = String::new();
let mut cnt = 0;
for diff in changeset.diffs {
match diff {
Difference::Same(ref x) => out.push_str(x),
Difference::Add(ref x) => out.push_str(&format!("\x1b[32m『{}』\x1b[0m", x)),
Difference::Rem(ref x) => out.push_str(&format!("\x1b[31m『{}』\x1b[0m", x)),
}
cnt += 1;
}
println!("--- {} differences found ---", cnt);
println!("{}", out);
println!("--- generated code ---");
println!("{}", compiled);
println!("=======================================");
panic!("Unexpected difference");
Copy link
Member

Choose a reason for hiding this comment

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

Can we also move out this part to a separate function?

@Mte90
Copy link
Member

Mte90 commented Jul 9, 2024

@FedericoCeratto can you check this PR?

echo "Hi, I'm {name}. I'm {age} years old."
"#,
r#"
__0_count=1
Copy link
Member

Choose a reason for hiding this comment

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

okay but this will be a pain in the ass to maintain.

compiled output should be expected to change as features rolled out, as well as there have been talks to simplify emitted variable names, remove awk and bc dependency and #72

Copy link
Member

Choose a reason for hiding this comment

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

@b1ek the point is to have just a few of these tests. This way we know if something does not blow out of the water. But I agree that this is something that is more suited for the stable release

Copy link
Member

Choose a reason for hiding this comment

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

so maybe we can do a minimum of tests?
Like for importing and echo?

@Ph0enixKM Ph0enixKM added this to the Stable release milestone Jul 9, 2024
@Mte90
Copy link
Member

Mte90 commented Jul 16, 2024

@FedericoCeratto just a ping

@Ph0enixKM
Copy link
Member

@FedericoCeratto @amber-lang/trusted-maintainers I'll close this PR for now so that it does not take space in open PR list. We can always reopen it

@Ph0enixKM Ph0enixKM closed this Nov 20, 2024
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.

4 participants