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

Figure out a testing story. #4

Open
emilio opened this issue May 13, 2020 · 9 comments
Open

Figure out a testing story. #4

emilio opened this issue May 13, 2020 · 9 comments

Comments

@emilio
Copy link
Collaborator

emilio commented May 13, 2020

We should figure out how do we want bikeshed-rs to be tested.

This probably involves:

  • Looking into how upstream bikeshed is tested. If we can come up with a way to share the tests that'd be best.

  • Decide whether we want to use regular #[test] functions, or something more like what cbindgen and bindgen use, where build.rs basically reads a tests/ folder and auto-generates the relevant cargo tests: https://github.com/eqrion/cbindgen/blob/master/build.rs

@annevk
Copy link
Collaborator

annevk commented May 13, 2020

See https://github.com/tabatkins/bikeshed/tree/master/tests. The idea is, given an input .bs, does it generate the expected .html.

@whichxjy
Copy link
Contributor

I use the serialize method of kuchiki to serialize the DOM tree now, but there's a special HTML serializer in bikeshed. If we want to use the expected HTML files in bikeshed for testing, maybe we need to write a serializer like that.

@emilio
Copy link
Collaborator Author

emilio commented May 25, 2020

Alternatively, can we parse the bikeshed HTML with kuchiki, and serialize it back to guarantee it's idempotent?

Output doesn't have to be whitespace-compatible IMO.

@whichxjy
Copy link
Contributor

That makes sense if there's nothing special about the serializer in bikeshed. What about building DOM trees from source files and expected HTML files, and then comparing them without serialization?

@annevk
Copy link
Collaborator

annevk commented May 26, 2020

That's also fine, but you'll still run into the whitespace issues I suspect. And potentially attribute ordering issues as well. (Attribute order is unfortunately still implementation-defined.)

@emilio
Copy link
Collaborator Author

emilio commented May 26, 2020

Yeah, however those issues should be easy to work around / ignore in the comparison (sorting the attributes, and skipping child whitespace text nodes where appropriate).

So it seems pretty reasonable to me.

@whichxjy
Copy link
Contributor

whichxjy commented Jun 9, 2020

I'm trying to compare DOM trees in this way:

fn is_equal(lhs: &NodeRef, rhs: &NodeRef) -> bool {
    if lhs.data() != rhs.data() {
        return false;
    }

    let lhs_children = lhs.children().collect::<Vec<NodeRef>>();
    let rhs_children = rhs.children().collect::<Vec<NodeRef>>();

    if lhs_children.len() != rhs_children.len() {
        return false;
    }

    for (lc, rc) in lhs_children.iter().zip(rhs_children.iter()) {
        if !is_equal(lc, rc) {
            return false;
        }
    }

    true
}

But it isn't working because whitespace characters that are outside of HTML elements will be treated as text nodes (kuchiki will treat newline character as NodeRef(Text(RefCell { value: "\n" })).

  • If we want to do the DOM-based comparison, we need to handle these text nodes in DOM trees.

  • If we want to do the string-based comparison, we need to ensure the generated HTML files and the expected HTML files have the same format.

@annevk
Copy link
Collaborator

annevk commented Jun 9, 2020

DOM-based comparison seems a little more flexible so you can document in code what kind of whitespace or attribute differences are acceptable. If you do string-based you might run into issues such as the hashmap ordering of attributes being different.

@whichxjy
Copy link
Contributor

The DOM-based comparison is working now. See #12.

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

No branches or pull requests

3 participants