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 way to not require skeptic for users of a crate using skeptic #60

Open
luser opened this issue Sep 5, 2017 · 13 comments
Open

Comments

@luser
Copy link

luser commented Sep 5, 2017

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
Copy link
Owner

budziq commented Sep 5, 2017

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
Copy link
Author

luser commented Sep 5, 2017

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
Copy link
Owner

budziq commented Sep 5, 2017

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
Copy link

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
Copy link
Owner

budziq commented Oct 10, 2017

@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
Copy link

@budziq yes, I was quite grumpy. The blocker was asajeffrey/swapper#3 (comment)

  Dev-dependencies are not allowed to be optional: `skeptic`

@nox
Copy link

nox commented Nov 8, 2017

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

@asajeffrey
Copy link

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

@budziq
Copy link
Owner

budziq commented Nov 12, 2017

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:

  • conflicting deps version handling
  • template/skt files

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
Copy link

@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
Copy link
Author

luser commented Dec 21, 2017

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.

im-0 added a commit to im-0/log4rs-syslog that referenced this issue Jan 15, 2018
im-0 added a commit to im-0/log4rs-syslog that referenced this issue Jan 15, 2018
epage added a commit to epage/liquid-rust that referenced this issue Apr 10, 2018
Markdown parsers and more are foited onto clients because of Skeptic.
This is inspired by the conversation at budziq/rust-skeptic#60 regarding
a "skeptic-lite" using rustdoc.
epage added a commit to epage/liquid-rust that referenced this issue Apr 10, 2018
Markdown parsers and more are foited onto clients because of Skeptic.
This is inspired by the conversation at budziq/rust-skeptic#60 regarding
a "skeptic-lite" using rustdoc.

Apparently, the stable version of rustdoc needs newlines between the
preceding paragraph and the code-block.  See rust-lang/rust#48068.
Eijebong added a commit to Eijebong/lettre that referenced this issue Apr 11, 2018
Unfortunately skeptic pulls all its dependencies in projects using
lettre (see budziq/rust-skeptic#60).
Eijebong added a commit to Eijebong/lettre that referenced this issue Apr 11, 2018
Unfortunately skeptic pulls all its dependencies in projects using
lettre (see budziq/rust-skeptic#60).
Eijebong added a commit to Eijebong/lettre that referenced this issue Apr 11, 2018
Unfortunately skeptic pulls all its dependencies in projects using
lettre (see budziq/rust-skeptic#60).
Eijebong added a commit to Eijebong/lettre that referenced this issue Apr 11, 2018
Unfortunately skeptic pulls all its dependencies in projects using
lettre (see budziq/rust-skeptic#60).
Eijebong added a commit to Eijebong/lettre that referenced this issue Apr 11, 2018
…calls

Unfortunately skeptic pulls all its dependencies in projects using
lettre (see budziq/rust-skeptic#60).
Eijebong added a commit to Eijebong/lettre that referenced this issue Apr 11, 2018
Unfortunately skeptic pulls all its dependencies in projects using
lettre (see budziq/rust-skeptic#60).
Eijebong added a commit to Eijebong/lettre that referenced this issue Apr 11, 2018
Unfortunately skeptic pulls all its dependencies in projects using
lettre (see budziq/rust-skeptic#60).
@epage
Copy link

epage commented Apr 17, 2018

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 im-0/log4rs-syslog#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.

rasky added a commit to rasky/lz4-rs that referenced this issue Dec 25, 2018
Similar to skpetic, but it does not add itself as a build
dependency, which means that all downstream users of LZ4
don't need to build skeptic (and all its dependencies).
See budziq/rust-skeptic#60.
@epage
Copy link

epage commented Mar 8, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants