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

Tutorial 1 and Tutorial 2 #5

Merged
merged 3 commits into from
Nov 3, 2022
Merged

Tutorial 1 and Tutorial 2 #5

merged 3 commits into from
Nov 3, 2022

Conversation

dynamite-bud
Copy link
Contributor

Added the two following tutorials

  • Strings and lists | tutorial_02
  • Resources | tutorial_05

Known Issues

When executing the cargo wapm I encounter the following error

2022-11-02T13:35:21.265039Z  INFO publish: cargo_wapm: Publishing dry_run=false pkg="hello-world"
Error: Unable to publish "hello-world"

Caused by:
    0: Unable to deserialize the [metadata] table
    1: missing field `wit-bindgen`

hello-world.waifile

[package]
name = "hello-world"
description = "Add two numbers"
authors.workspace = true
edition.workspace = true
homepage.workspace = true
license.workspace = true
readme.workspace = true
repository.workspace = true
rust-version.workspace = true
version.workspace = true

[package.metadata.wapm]
namespace = "wasmer"  # Replace this with your WAPM username
abi = "none"
bindings = { wai-bindgen = "0.1.0", exports = "hello-world.wai" }

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
wai-bindgen-rust = "0.2.0-rc.1"

[lib]
crate-type = ["cdylib", "rlib"]

@@ -1,5 +1,5 @@
[package]
name = "wasmer-pack"
name = "hello-world"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call this tutorial-01 to be consistent with what the tutorial says and the name folder name.

@@ -0,0 +1,25 @@
[package]
name = "strings-and-lists"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you call this tutorial-02 as well? Might also want to rename the folder from tutorial_02 to tutorial-02 to match the tutorial.

Comment on lines 39 to 41
let people = ["Rudra", "Michael", "Syrus"]
.map(|person| person.to_string())
.to_vec();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the vec![] macro to construct a Vec directly.

Suggested change
let people = ["Rudra", "Michael", "Syrus"]
.map(|person| person.to_string())
.to_vec();
let people = vec!["Rudra".to_string(), "Michael".to_string(), "Syrus".to_string()];

Comment on lines 42 to 49
let answer = match people.as_slice() {
[] => "Oh, nobody's there...".to_string(),
[person] => format!("Hello, {person}!"),
[people @ .., last] => {
let people = people.join(", ");
format!("Hello, {people}, and {last}!")
}
};
Copy link
Contributor

Choose a reason for hiding this comment

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

For tests, I would prefer to hard-code the answer instead of copy/pasting the implementation we're testing.

After all, if the original match is wrong and our test just copy/pastes it, the test will pass even though we've got bad logic.

Suggested change
let answer = match people.as_slice() {
[] => "Oh, nobody's there...".to_string(),
[person] => format!("Hello, {person}!"),
[people @ .., last] => {
let people = people.join(", ");
format!("Hello, {people}, and {last}!")
}
};
let answer = "Rudra, Michael, and Syrus".to_string();

@@ -0,0 +1,25 @@
[package]
name = "resources"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name = "resources"
name = "tutorial-05"

[package.metadata.wapm]
namespace = "wasmer" # Replace this with your WAPM username
abi = "none"
bindings = { wai-bindgen = "0.1.0", exports = "resources.wai" }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bindings = { wai-bindgen = "0.1.0", exports = "resources.wai" }
bindings = { wit-bindgen = "0.1.0", exports = "resources.wai" }

Comment on lines 43 to 48
let num1 = 5;
let num2 = 10;
calulator.add(num1 as f32);
calulator.add(num2 as f32);
let result = calulator.current_value();
let answer = 5.0 + (num1 + num2) as f32;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can get rid of these as f32 casts by writing 5.0.

Suggested change
let num1 = 5;
let num2 = 10;
calulator.add(num1 as f32);
calulator.add(num2 as f32);
let result = calulator.current_value();
let answer = 5.0 + (num1 + num2) as f32;
calulator.add(5.0);
calulator.add(10.0);
let result = calulator.current_value();
let answer = 5.0 + (5.0 + 10.0);

There are different opinions on this, but normally I'd prefer to copy a variable in my tests rather than put them behind a variable (e.g. num2).

My thinking is the way you approach test code should be different to how you approach normal code because the way they are used is different. You'll want less abstraction during testing because when you need to fix the test 3 months down the track, you don't want to mentally unwrap the abstractions to figure out what the test does.


#[test]
fn check_add() {
let calulator = Calculator::new(5.0);
Copy link
Contributor

Choose a reason for hiding this comment

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

For this test, let's try to throw in some very different numbers (e.g. initial value of 0.51, num1 = 1.0, num2 = 10.0). That way, the sum we're calculating is 0.5 + (1.0 + 10.0) = 11.5, and it's obvious where every bit of the result came from.

With the result of 20.0, there are multiple ways you can get there (e.g. 2*num1 + num2, num2 * num_times_add_was_called, etc.), but by using very different numbers it's much less likely that we'd get a test that accidentally passes.

It's not so much of an issue with the Calculator type because we can see the implementation and know it's trivial, but I've been bitten before when my code had two variables the wrong way around, but my test still passed because their value was the same.

Footnotes

  1. Note: I used 0.5 here instead of 0.1 to avoid floating point weirdness.

Comment on lines 68 to 80
let calulator = Calculator::new(1.0);
let num = 6;
let mut answer = 1;
for i in 1..=num {
calulator.multiply(i as f32);
answer *= i;
}
let answer = answer as f32;
let result = calulator.current_value();
assert_eq!(
result, answer,
"The multiply function is not working properly"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not immediately obvious what this code is testing because you've mixed the code under test (calculator.multiply()) in with code that prepares answer. I'd much rather you used a hard-coded value or just did let answer = (1.0_f32..6.0).product() at the end.

When I write tests, I normally try to split it into three obvious phases - setting up, running the code to be tested, and assertions.

Here's a good example:

    #[test]
    fn package_json() {
        let metadata = Metadata::new("wasmerio/wasmer-pack".parse().unwrap(), "0.0.0");

        let got = generate_package_json(false, &metadata);

        insta::assert_display_snapshot!(got.utf8_contents().unwrap());
    }

    #[test]
    fn package_json_wasi() {
        let metadata = Metadata::new("wasmerio/wabt".parse().unwrap(), "0.0.0");

        let got = generate_package_json(true, &metadata);

        insta::assert_display_snapshot!(got.utf8_contents().unwrap());
    }

let answer = answer as f32;

// Multiply till 6
(1..=num).for_each(|num| calulator.multiply(num as f32));
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan Nov 3, 2022

Choose a reason for hiding this comment

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

Can you use a for-loop here? Generally, Rust prefers for for-loops while for_each() is more of a JavaScript-ism.


// Multiply till 6
(1..=num).for_each(|num| calulator.multiply(num as f32));
let answer = (1..=6).fold(1, |a, b| a * b) as f32;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using Iterator::product() will make this more readable than fold().

@dynamite-bud dynamite-bud marked this pull request as ready for review November 3, 2022 09:00
@dynamite-bud dynamite-bud merged commit 8441cd2 into main Nov 3, 2022
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.

2 participants