High level integration tests #374

Michael-F-Bryan · 2017-07-08T20:37:58Z

To make the job of merging #371 in with the rest of MDBook a bit easier I thought I'd add some really high level integration tests. These serve more as "sanity tests" than anything else. They're meant to exercise the entire library as a whole, doing things like:

Making sure the SUMMARY.md is parsed correctly and you can build a valid book
Check that doc tests get run and detect failing tests
Makes sure mdbook init will create a new book directory with sane defaults
Ensure links between rendered chapters are correct and other easily testable things in the rendered html

Michael-F-Bryan · 2017-07-09T09:45:49Z

I can't think of any more integration tests which can be used to check mdbook, @budziq or @azerupi would you be able to flick through this let me know what you think?

budziq

@Michael-F-Bryan I'll try to look closer and run these tests on Monday when I have access to PC. On cursory glance lgtm 😃

budziq · 2017-07-09T12:51:19Z

tests/integration_tests.rs

+/// Read the contents of the provided file into memory and then iterate through
+/// the list of strings asserting that the file contains all of them.
+fn assert_contains_strings<P: AsRef<Path>>(filename: P, strings: &[&str]) {
+    println!("Checking {}", filename.as_ref().display());


do we really need the tests to write to stdout?
also why the need for freestanding println!();? println! is able to take any format string including newlines

This is for debugging. Basically when the test fails you won't see the full context of the failure, which in this case would include the file name it failed for, the contents of the file, and the text it's looking for. Normally stdout will get thrown away on a passing test, but when the test fails it'll show you all the information printed out and make identifying why the test failed a lot easier.

I guess you could include those in assert!()'s panic message, I've just done it this way out of habit. I can easily swap to printing everything in assert message if you want, should I do that?

Right analyzing the code on mobile is not ideal ;). In general I prefer plain asserts but I'm good either way in this case. But please fix the empty printlns. I'll check the rest later this week.

Yeah, I usually prefer plain asserts too however after writing out two or three I found I was copy/pasting the exact same set of assertions for each test and pulled it out into a function to make it easier to understand and read the tests.

Yeah, looks like those println!() statements will need to go.

azerupi · 2017-07-09T13:26:37Z

Can we separate the tests into multiple files each testing one specific domain?
I am asking because we will want to add a lot more of such tests in the future for existing and new functionality and it would quickly become chaotic if we need to write everything in one file.

I don't recall how cargo handles integration tests. If I remember correctly, cargo treats each file in the tests/ directory as a separate unit? Is it possible to have a file containing all the setup functions and then one file per feature / module testing specific things?

I haven't played with tests in Rust enough to answer those questions.

budziq · 2017-07-09T13:31:21Z

Is it possible to have a file containing all the setup functions and then one file per feature / module testing specific things?

You can import any module, common file can live even in the /tests dir.

Michael-F-Bryan · 2017-07-09T13:40:26Z

You can import any module, common file can live even in the /tests dir.

Oh nice! I was planning to do some ugly stuff with include!() to "import" the helper stuff and macros to ensure setup and teardown gets done properly, but that feels a lot nicer.

I don't particularly mind having to do let temp = create_book(true); at the top of each test. With the existing setup I've made sure the setup is only a single line so it's not overly painful, and teardown gets done correctly because TempDir deletes itself on drop.

Also, can you guys think of any more integration tests which should be added to this PR? I'm looking for things which are easy to determine with something like assert!(text.contains(some_string)). I've already added tests to make sure the rendered content contains links to all the other files as well as checking if a class="playpen" gets added to code snippets. Let me know if there's any other low hanging fruit we can add to the test suide.

Michael-F-Bryan · 2017-08-02T13:10:27Z

@azerupi, any chance you can review this and let me know what you think of it or if any of the tests could be written better?

Once this base PR is merged, adding more integration/regression tests on top of it will be a good target for new contributors. Instead of adding more tests to the PR I think we could create separate issues and post them on the TWIR Call For Participation thread. How does that sound?

azerupi · 2017-08-02T13:20:45Z

Yes, I'm checking it out now :)

Instead of adding more tests to the PR I think we could create separate issues and post them on the TWIR Call For Participation thread.

Absolutely, tests would make very simple and isolated first issues. Especially if the ground work is already done and a couple of examples are present.

azerupi · 2017-08-02T13:42:25Z

tests/helpers.rs

+use tempdir::TempDir;
+
+
+const SUMMARY_MD: &'static str = "# Summary


Just an idea for maintainability:

We could make a simple directory with a dummy book like:

tests ├── config.rs ├── helpers.rs ├── init.rs ├── jsonconfig.rs ├── rendered_output.rs ├── test-book │ ├── SUMMARY.md │ ├── conclusion.md │ ├── first │ │ ├── index.md │ │ └── nested.md │ └── second.md ├── testing.rs └── tomlconfig.rs

and then include the files like this

static SUMMARY: &str = include_str!("test-book/SUMMARY.md");

It makes it easier to edit and expand the book when we add more tests.

Actually, since you convert to bytes later you can directly use ìnclude_bytes!()

azerupi · 2017-08-02T13:53:46Z

tests/helpers.rs

+/// `assert!($TEST_STATUS)`. If you want to check MDBook's testing
+/// functionality, `$TEST_STATUS` can be substitute for either `true` or
+/// `false`. This is done using the `passing_test` parameter.
+pub fn create_book(passing_test: bool) -> TempDir {


I am a little worried that we will end up with a huge number of arguments for this function in the future.. Currently there is only one argument passing_test, but as more tests are added I am afraid that more arguments will appear too.

Maybe we should make this a builder struct with a good set of default options? In the simplest case we will have to call BookBuilder::create() but in more complex scenarios, when we want some variation, we can just pass the arguments that need changing. What do you think?

I like the BookBuilder idea! It'll make things a lot easier in the long run, and also integrates quite nicely with your earlier dummy book directory comment.

azerupi · 2017-08-02T14:03:45Z

tests/init.rs

+
+
+#[test]
+fn run_mdbook_init() {


Maybe you could add one or two lines of comments to explain what each test does?
It's easier if people don't have to read the code to understand what exactly the test is asserting.

Good point. run_mdbook_init is a pretty useless name. I'll rename it to something longer like base_mdbook_init_should_create_default_content.

azerupi · 2017-08-02T14:07:17Z

tests/init.rs

+}
+
+#[test]
+fn run_mdbook_init_with_custom_args() {


maybe the name can reflect that it specifically tests the source and output destination?

azerupi

I reviewed the changes. This is a really great PR 🎉

I left a couple of comments for possible improvements. I think the names of the tests do not always reflect what they are testing. But that can be changed later.

Michael-F-Bryan · 2017-08-02T14:36:45Z

I pulled the dummy book generation out into a DummyBook builder and refactored its build() method to use a table of (PathBuf, &str) to populate the temporary directory instead of the copy/paste approach I was using before.

Hopefully the names will be a bit easier to understand as well. I was probably being lazy when naming them before, assuming anyone checking up on a failing test would have the test in front of them and would understand it at a glance.

azerupi · 2017-08-02T14:44:42Z

Perfect!
I will merge once either Travis or AppVeyor turns green.

Thanks a lot!

budziq · 2017-08-02T15:03:57Z

I didn't have time to really look into these changes (sorry :( ) but these look great! Excellent Work 💯 @Michael-F-Bryan

High level integration tests

azerupi added the A-Tests Area: `mbdook test` related tests label Jul 9, 2017

Michael-F-Bryan added 5 commits July 9, 2017 19:59

Added some end-user "sanity" tests

9fab267

Added playpen and mdbook init tests

e3f047a

Made sure the rendered content actually contains the original text

8b7c95e

trivial change to make travis run again

cd11035

Updated the book testing to take into account rust-lang#340

c90c0f7

Michael-F-Bryan force-pushed the moar-tests branch from a4f1a05 to c90c0f7 Compare July 9, 2017 12:02

budziq reviewed Jul 9, 2017

View reviewed changes

Michael-F-Bryan changed the title ~~High level integration tests~~ [WIP]: High level integration tests Jul 9, 2017

Michael-F-Bryan added 3 commits July 9, 2017 22:08

Bye bye println() debugging

7e8819f

Broke the integration tests out into individual sections

29e00c0

Tested MDBook::init with custom args

0c3a2b8

Michael-F-Bryan changed the title ~~[WIP]: High level integration tests~~ High level integration tests Jul 11, 2017

Michael-F-Bryan mentioned this pull request Jul 15, 2017

Book representation #371

Closed

Michael-F-Bryan mentioned this pull request Jul 31, 2017

Introduce smoke test #237

Closed

azerupi reviewed Aug 2, 2017

View reviewed changes

Got some feedback from azerupi and added a DummyBook builder

e2eb40b

azerupi approved these changes Aug 2, 2017

View reviewed changes

azerupi merged commit 6628757 into rust-lang:master Aug 2, 2017

Michael-F-Bryan deleted the moar-tests branch August 21, 2017 03:39

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020

Merge pull request rust-lang#374 from Michael-F-Bryan/moar-tests

3688496

High level integration tests

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

High level integration tests #374

High level integration tests #374

Michael-F-Bryan commented Jul 8, 2017

Michael-F-Bryan commented Jul 9, 2017

budziq left a comment •

edited

Loading

budziq Jul 9, 2017

Michael-F-Bryan Jul 9, 2017

budziq Jul 9, 2017

Michael-F-Bryan Jul 9, 2017

azerupi commented Jul 9, 2017

budziq commented Jul 9, 2017

Michael-F-Bryan commented Jul 9, 2017

Michael-F-Bryan commented Aug 2, 2017

azerupi commented Aug 2, 2017

azerupi Aug 2, 2017

azerupi Aug 2, 2017

azerupi Aug 2, 2017

Michael-F-Bryan Aug 2, 2017

azerupi Aug 2, 2017

Michael-F-Bryan Aug 2, 2017

azerupi Aug 2, 2017

azerupi left a comment

Michael-F-Bryan commented Aug 2, 2017

azerupi commented Aug 2, 2017

budziq commented Aug 2, 2017

		use tempdir::TempDir;


		const SUMMARY_MD: &'static str = "# Summary



		#[test]
		fn run_mdbook_init() {

High level integration tests #374

High level integration tests #374

Conversation

Michael-F-Bryan commented Jul 8, 2017

Michael-F-Bryan commented Jul 9, 2017

budziq left a comment • edited Loading

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

azerupi commented Jul 9, 2017

budziq commented Jul 9, 2017

Michael-F-Bryan commented Jul 9, 2017

Michael-F-Bryan commented Aug 2, 2017

azerupi commented Aug 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azerupi left a comment

Choose a reason for hiding this comment

Michael-F-Bryan commented Aug 2, 2017

azerupi commented Aug 2, 2017

budziq commented Aug 2, 2017

budziq left a comment •

edited

Loading