Regression tests #422

Michael-F-Bryan · 2017-09-03T08:34:44Z

I've started writing a bunch of tests which build the example book and check to see "significant" parts are still present. This should help us with regression testing because if something is accidentally broken with in the rendered output, Travis will be able to pick it up. Similar to @steveklabnik's #237.

At the moment I'm just checking the TOC structure, but I'll keep adding other things. I want to also check samples from random chapters are present in the output, and make sure things like the playground buttons and next/previous links are present.

This'll hopefully help with #409 because I know I've broken at least some of the output. It does add some maintenance cost though, seeing as when we update the example book or the rendered output you may need to revisit these tests and update them.

Michael-F-Bryan · 2017-09-14T16:31:03Z

@azerupi or @budziq, any chance you can give this a quick review? It looks like I've let it bitrot a bit, but it'll be nice to know if I'm going in the right direction :)

budziq · 2017-09-15T13:40:46Z

tests/regression.rs

+use select::predicate::{Class, Name, Predicate};
+
+
+const BOOK_ROOT: &'static str = concat!(env!("CARGO_MANIFEST_DIR"), "/book-example");


I would suggest to using something more stable than the book-example. It is our de facto documentation which will naturally grow and having it rigorously tested against static tests might make a little painful to edit.
How about using something a little more synthetic and static with which we could test several ugly edge-cases and for instance check if:

playpens with given info-strings results in required css classes

if js libs are imported if given option in book.toml is given.

???

Actually, we might consider using some non rust solutions for web regression testing (I'm no webdev but I've seen some really impressive magic done with selenium)

Yeah, my first thought was to use Python and BeautifulSoup because it makes it loads easier to navigate HTML documents and find the things you want, I was a little hesitant to add more complication and a second language to the testing process though. I'll have a look at what other projects do to verify rendered HTML/CSS and check for regressions.

In terms of making things more static, would it be a good idea to rewrite this based on the "dummy book" instead? It'd mean the integration tests need to be updated whenever you change it so you can test for regressions, but that should give a nicer experience than tests breaking whenever we update documentation.

There are libraries similar to BeautifulSoup in Rust

In terms of making things more static, would it be a good idea to rewrite this based on the "dummy book" instead?

Yeah. Im not even sure we need to create it dynamically. Why not have the dummy book as a directory structure fixture similar to book-example which we could easily expand

There are libraries similar to BeautifulSoup in Rust

These tests already use the select (nice wrapper over html5ever) crate which is as close to bs4 as we can get now. I was thinking more in terms of actual usability tests with headless selenium but might be too ambitious for now.

Why not have the dummy book as a directory structure fixture similar to book-example which we could easily expand

I like this idea. Originally I was dynamically generating the dummy book in a temporary directory so each test is completely independent from the others, but there's no reason why we can't just do a cp -r into a temp directory.

Michael-F-Bryan · 2017-09-15T19:39:55Z

@steveklabnik, do you have some sort of link checker or other method for checking rendered documentation in The Rust Book?

I'm trying to think of various ways we can do sanity checks to make sure a change in mdbook doesn't accidentally break rendered documents without us noticing.

steveklabnik · 2017-09-15T20:40:42Z

It's here: https://github.com/rust-lang/rust/tree/master/src/tools/linkchecker

There's talk of putting it on crates.io, but I don't know if that's happened.

Michael-F-Bryan · 2017-09-30T09:29:31Z

Note to self: the regression tests need to be rewritten so they depend upon a dummy book instead of the example documentation.

Also rewrite the dummy and helper modules so creating a new copy of the dummy book is a simple cp -r to a temporary directory instead of the current include_str!() madness. I'd still like to make sure each regression test uses its own copy of the dummy book in a temp dir though. That way tests can't accidentally overwrite the original or interfere with each other.

Michael-F-Bryan · 2017-11-11T18:49:36Z

I revised this so it now exercises a contrived "dummy book" instead of the example book we use as the de-facto documentation for mdbook. So far it's just checking the TOC is correct and that a bunch of known links and CSS classes are present in files.

It's by no means complete, but now the foundations are established we can add to the regression tests over time when people report bugs. @budziq or @steveklabnik, any chance you can skim through this and let me know what you think?

The regression tests themselves are in tests/rendered_output.rs. There's a bit of code churn because I renamed tests/dummy to tests/dummy_book to make it clearer this is a contrived book for testing.

budziq · 2017-11-12T22:21:42Z

budziq or steveklabnik, any chance you can skim through this and let me know what you think?

@Michael-F-Bryan Wow you're going fast :)
I most likely will not be able to keep up with the pace but will try to find some time next week (sadly I could steal only few minute timeslices this weekend)

Michael-F-Bryan · 2017-11-13T10:00:29Z

@Michael-F-Bryan Wow you're going fast :)

I think it was just good timing in that I had the entire weekend free when things started happening 😃 I'm not sure how much I'll get done during the week due to work and all that though.

I'm also pretty keen to get the Book refactoring PR back in motion, but that depends on this PR so I can detect if I accidentally broke stuff... Which I have a sneaking suspicion that I have.

budziq

@Michael-F-Bryan Overall It look ok but I would suggest few changes:

budziq · 2017-11-13T14:23:35Z

tests/dummy_book/mod.rs

+
+
+/// Recursively copy an entire directory tree to somewhere else (a la `cp -r`).
+fn recursive_copy<A: AsRef<Path>, B: AsRef<Path>>(from: A, to: B) -> Result<(), Box<Error>> {


we already use error_chain how about we use it also in this context instead of boxing the errors?
also
copy_files_except_ext already exists

budziq · 2017-11-13T14:26:27Z

tests/dummy_book/mod.rs

+    Ok(())
+}
+
+pub fn read_file<P: AsRef<Path>>(filename: P) -> Result<String, Box<Error>> {


there is already a file_to_string

budziq · 2017-11-13T14:32:53Z

tests/dummy_book/mod.rs

+    let filename = filename.as_ref();
+
+    let mut content = String::new();
+    File::open(&filename).expect("Couldn't open the provided file")


we can use the file_to_string fn from utils

budziq · 2017-11-13T14:36:33Z

tests/dummy_book/mod.rs

+    /// If this fails for any reason it will `panic!()`. If we can't write to a
+    /// temporary directory then chances are you've got bigger problems...
+    pub fn build(&self) -> TempDir {
+        let temp = TempDir::new("dummy_book").unwrap();


unwraps trigger my OCD ;) (and I tend to grep for them as TODO's) we already have error_chain so using -> Result<()> is trivial or maybe we could at least use expects.

Good point, I think I was just being lazy and not wanting to deal with errors properly. It does just end up pushing the unwrap()'s up one level to the tests though...

budziq · 2017-11-13T14:37:55Z

tests/dummy_book/mod.rs

+    }
+}
+
+fn poor_mans_sed(filename: &Path, from: &str, to: &str) {


the name is not super descriptive. I'd rename it to replace_file_contents

budziq · 2017-11-13T14:49:46Z

tests/rendered_output.rs

+/// can search with the `select` crate
+fn root_index_html() -> Document {
+    let temp = DummyBook::new().build();
+    MDBook::new(temp.path()).build().unwrap();


how about we return Result<Document>?

budziq · 2017-11-13T15:01:33Z

tests/rendered_output.rs

+
+    let pred = descendants!(Class("chapter"), Name("li"), Name("li"), Name("a"));
+
+    let mut children_of_children: Vec<String> =


Vec<_> type ascription should be enough

budziq · 2017-11-13T15:01:38Z

tests/rendered_output.rs

+
+    let pred = descendants!(Class("chapter"), Name("li"), Name("a"));
+
+    let mut children: Vec<String> = doc.find(pred).map(|elem| elem.text().trim().to_string())


Vec<_> type ascription should be enough

budziq · 2017-11-13T15:04:30Z

tests/dummy_book/mod.rs

+                          .unwrap();
+}
+
+impl Default for DummyBook {


do we need a separate Default impl? Why not just implement new directly?

budziq · 2017-11-13T15:05:12Z

tests/dummy_book/mod.rs

+
+impl DummyBook {
+    /// Create a new `DummyBook` with all the defaults.
+    pub fn new() -> DummyBook {


why not implement it directly? It's a single bool.

Michael-F-Bryan · 2017-11-14T11:42:56Z

@budziq I went back through and added better error handling to the DummyBook mock, now it does proper error propagation and adds more high level context using chain_err().

What do you usually do in test code when the setup stages fail? For example, there are quite a few tests which look like the example below. Do you reckon it's fine to just leave the unwrap() as-is (the idea being that our error chain should give sufficient context), or would you usually use an expect() instead?

#[test]
fn chapter_files_were_rendered_to_html() {
    let temp = DummyBook::new().build().unwrap();

    // do stuff and make assertions
}

budziq · 2017-11-15T16:43:18Z

@Michael-F-Bryan

What do you usually do in test code when the setup stages fail?

IMHO panic is the best option but I usually prefer to do it at top level if possible or at least with expects instead of unwraps.

budziq

@Michael-F-Bryan looks much nicer now! Unless you are aiming for more changes I'd squash and merge.

Michael-F-Bryan · 2017-11-16T07:09:52Z

@budziq, any idea why the beta mac build is failing? I thought it might just be a temporary thing due to a flaky build, but restarting it gave the exact same error message.

https://travis-ci.org/rust-lang-nursery/mdBook/jobs/301898364

@budziq

* Created regression tests for the table of contents * Refactoring to make the test more readable * Fixed some bitrot and removed the (now redundant) tests/helper module * Removed the include_str!() stuff and use just the dummy book for testing * Regression tests now pass again! * Pinned a `*` dependency to use a particular version * Made sure test mocks return errors instead of panicking * Addressed the rest of @budziq's review * Replaced a file open/read with file_to_string

Michael-F-Bryan force-pushed the regression-tests branch from 2d5e84f to d9d634c Compare September 14, 2017 16:49

budziq reviewed Sep 15, 2017

View reviewed changes

Michael-F-Bryan added 3 commits November 12, 2017 02:10

Created regression tests for the table of contents

95609cf

Refactoring to make the test more readable

d11e3f6

Fixed some bitrot and removed the (now redundant) tests/helper module

dc9c1c9

Michael-F-Bryan force-pushed the regression-tests branch from d9d634c to dc9c1c9 Compare November 11, 2017 18:13

Michael-F-Bryan added 2 commits November 12, 2017 02:34

Removed the include_str!() stuff and use just the dummy book for testing

d86f960

Regression tests now pass again!

f8a3382

Pinned a * dependency to use a particular version

4266a93

budziq suggested changes Nov 13, 2017

View reviewed changes

Michael-F-Bryan added 3 commits November 14, 2017 19:17

Made sure test mocks return errors instead of panicking

1bcd914

Addressed the rest of @budziq's review

65191c3

Replaced a file open/read with file_to_string

2a1ae82

budziq approved these changes Nov 15, 2017

View reviewed changes

Michael-F-Bryan merged commit d56ff94 into rust-lang:master Nov 16, 2017

Michael-F-Bryan deleted the regression-tests branch November 18, 2017 11:07

This was referenced Nov 18, 2017

Travis builds failing for OSX & beta #490

Closed

Refactor navigation helpers #465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Regression tests #422

Regression tests #422

Michael-F-Bryan commented Sep 3, 2017

Michael-F-Bryan commented Sep 14, 2017

budziq Sep 15, 2017

Michael-F-Bryan Sep 15, 2017

steveklabnik Sep 15, 2017

budziq Sep 15, 2017

Michael-F-Bryan Sep 15, 2017

Michael-F-Bryan commented Sep 15, 2017

steveklabnik commented Sep 15, 2017

Michael-F-Bryan commented Sep 30, 2017

Michael-F-Bryan commented Nov 11, 2017

budziq commented Nov 12, 2017

Michael-F-Bryan commented Nov 13, 2017

budziq left a comment

budziq Nov 13, 2017

budziq Nov 13, 2017

budziq Nov 13, 2017

budziq Nov 13, 2017

Michael-F-Bryan Nov 14, 2017

budziq Nov 13, 2017

budziq Nov 13, 2017

budziq Nov 13, 2017

budziq Nov 13, 2017

budziq Nov 13, 2017

budziq Nov 13, 2017

Michael-F-Bryan commented Nov 14, 2017

budziq commented Nov 15, 2017

budziq left a comment

Michael-F-Bryan commented Nov 16, 2017

		use select::predicate::{Class, Name, Predicate};


		const BOOK_ROOT: &'static str = concat!(env!("CARGO_MANIFEST_DIR"), "/book-example");



		/// Recursively copy an entire directory tree to somewhere else (a la `cp -r`).
		fn recursive_copy<A: AsRef<Path>, B: AsRef<Path>>(from: A, to: B) -> Result<(), Box<Error>> {


		let pred = descendants!(Class("chapter"), Name("li"), Name("li"), Name("a"));

		let mut children_of_children: Vec<String> =


		let pred = descendants!(Class("chapter"), Name("li"), Name("a"));

		let mut children: Vec<String> = doc.find(pred).map(\|elem\| elem.text().trim().to_string())

Regression tests #422

Regression tests #422

Conversation

Michael-F-Bryan commented Sep 3, 2017

Michael-F-Bryan commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Sep 15, 2017

steveklabnik commented Sep 15, 2017

Michael-F-Bryan commented Sep 30, 2017

Michael-F-Bryan commented Nov 11, 2017

budziq commented Nov 12, 2017

Michael-F-Bryan commented Nov 13, 2017

budziq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Nov 14, 2017

budziq commented Nov 15, 2017

budziq left a comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Nov 16, 2017