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

Issue #56 - Use unique database filenames and cleanup the database files after testing. #61

Merged
merged 1 commit into from
Mar 11, 2016
Merged

Issue #56 - Use unique database filenames and cleanup the database files after testing. #61

merged 1 commit into from
Mar 11, 2016

Conversation

hfiguiere
Copy link
Contributor

No description provided.

@hfiguiere
Copy link
Contributor Author

@arcturus r?

@hfiguiere
Copy link
Contributor Author

Apparently "doc test" will still fail for the same reason.

@hfiguiere
Copy link
Contributor Author

This skip "running" the documentation examples.

@delapuente
Copy link
Contributor

Not only this makes tests more complicated but it introduces a way for examples to be out of date, reduce coverage (the examples themselves). The solution is to mock db or use one process. I would not accept a patch introducing complexity until the real problem is fixed.

@hfiguiere
Copy link
Contributor Author

What is complicated is that:

  1. you need more than just cargo build to make the test pass.
  2. that the test have persistent side effects due to the library and this is also a very high complexity.

What my patch do is remove 1. and address 2.

As for the real problem, issue #53 should solve the side effect better.

For the documentation examples:

  1. they are still built, just not run.
  2. due to the aforementioned side effects, considering them as reliable test is shooting oneself in the foot. Orange factor will be back with that sort of things.

@delapuente
Copy link
Contributor

I did not know about no_run. Thanks for pointing it.

@@ -344,6 +344,7 @@ describe! setup_tests {
assert!(false);
}
};
remove_test_db();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we move this to after_each ?

And all the remove_test_db calls below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I do that I seem to get a test failure. And I haven't figured out how to view the generated code by the macro - to see what was happening - so I chose the easy way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would I be really picky if i ask to spend some times knowing why it's happening? Cause it looks a bit weird to me right now with all those calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we are hitting reem/stainless#47.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a ; at the end of each "it" block make it work. Updating PR.

@hfiguiere
Copy link
Contributor Author

r? @arcturus - I addressed that last comment working around a bug in stainless.

@arcturus
Copy link
Collaborator

Great stuff!

arcturus added a commit that referenced this pull request Mar 11, 2016
Issue #56 - Use unique database filenames and cleanup the database files after testing.
@arcturus arcturus merged commit 69ac893 into fxbox:master Mar 11, 2016
@hfiguiere hfiguiere deleted the intermittent-tests branch March 11, 2016 14:03
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.

None yet

3 participants