budziq / rust-skeptic

Test your Rust Markdown documentation via Cargo
Apache License 2.0
285 stars 43 forks source link

Figure out a way to not require skeptic for users of a crate using skeptic #60

Open luser opened 7 years ago

luser commented 7 years ago

Using skeptic from build.rs and having it in build-dependencies means that everyone using a crate that uses skeptic winds up building skeptic and all of its dependencies, which adds build time and provides no real value to the end user. It would be nice to figure out a way to only require skeptic when building tests for a crate.

One way to support this would be if cargo added a way to run build scripts for tests. Another way would be for skeptic to be implemented as a procedural macro that could be invoked from inside a #[cfg(test)] block.

budziq commented 7 years ago

Hi,

Indeed, skeptic is built on top of few hacks glued together as there is no way to hook onto the test harness other than writing an actual rs file containing tests into some directory recognized by cargo. Also I am waiting for build plans to land in order not to be able to hook into cargo smarts about dependency resolution and compilation.

At this moment I see not way to fix these issues but I'm open to ideas!

luser commented 7 years ago

One way that I'm pretty sure could be made to work currently would be to use macros 1.1 as a hack for procedural macros, something like:

#[cfg(test)]
mod test {
  #[macro_use]
  extern crate skeptic_derive;

  #[derive(Skeptic)]
  #[skeptic_files = "README.md, docs/foo.md"]
  struct SkepticTests;
}

Obviously it'd be nicer to have real procedural macro support so it could look more like:

#[cfg(test)]
mod test {
  #[macro_use]
  extern crate skeptic;

  add_skeptic_tests!["README.md", "docs/foo.md"];
}
budziq commented 7 years ago

Hmm neat idea @luser! This would require fundamental skeptic rewrite and I cannot say that I'm well acquainted with 1.1 style macros, so none of these would happen in the foreseeable future (I'm currently the only active maintainer with not much spare time).

I would really welcome a PR or a partial POC if you would be willing to contribute some of your time.

asajeffrey commented 6 years ago

Annoyingly, this issue caused me to switch off skeptic for https://github.com/asajeffrey/swapper/ since it was dragging skeptic and all its dependencies into servo.

budziq commented 6 years ago

@asajeffrey That's sad to read but reasonable. This is probably the biggest problem with skeptic today. If you have any thoughts on the matter I'd be glad to read them.

asajeffrey commented 6 years ago

@budziq yes, I was quite grumpy. The blocker was https://github.com/asajeffrey/swapper/pull/3#issuecomment-316421198

  Dev-dependencies are not allowed to be optional: `skeptic`
nox commented 6 years ago

@luser's workaround means one cannot avoid a rebuild when running cargo test with only integration tests and no unit tests.

asajeffrey commented 6 years ago

@nox suggests a solution, which is to have a separate cargo command for running skeptic: https://mozilla.logbot.info/servo/20171108#c13824351

budziq commented 6 years ago

Hi @nox, @asajeffrey,

Sorry for taking so long to respond. Do you see any advantage of separate cargo skeptic or a standalone skeptic cli app over just running rustdoc?

rustdoc ./README.md --test -L ./target/debug/deps/

As far as I know these should be more or less equivalent except for:

I'm inclined to follow @luser's suggestion once I carve enough free time to actually play with Macros 1.1. This should make the lib slightly less painful to use.

If you see a clear advantage of cargo skeptic over rustdoc I might be persuaded otherwise.

asajeffrey commented 6 years ago

@budziq yes, I ended up just running rustdoc directly, e.g. https://github.com/asajeffrey/josephine/blob/e6a385af2ac2f83a2af21b634b840cd95de17662/.travis.yml#L15. The lack of .skt is a bit annoying, but it does the job!

luser commented 6 years ago

I liked @asajeffrey's suggestion so I tried out integrating it as a Rust test so that it'd run as part of cargo test: https://github.com/luser/read-process-memory/blob/master/tests/skeptic.rs

It seems to work reasonably well! It would be neat to figure out if that could be written as a macro so it could be encapsulated into some sort of skeptic-lite crate.

epage commented 6 years ago

While I know skeptic can be replaced with as little as rustdoc -L target/debug/deps/ --test README.md, I've gone ahead and created a "skeptic-lite" inspired from https://github.com/im-0/log4rs-syslog/pull/17

https://github.com/assert-rs/docmatic

I see some people have been adding --extern. So far I've not done that but we can always look at adding it in the future.

epage commented 5 years ago

Do you see any advantage of separate cargo skeptic or a standalone skeptic cli app over just running rustdoc?

So I'm not looking at this from the angle of integrating into a CI (test reporting, coverage, etc). So far we've been creating replacements for cargo test. I'm feeling ambitious right now and I'd like to add support for doctests and this got me thinking it'd be nice to have these tools also support the role of skeptic. This means we'd ship a pre-built binary that people download and run in their CI rather than having to make skeptic a dependency