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

Move unit tests into its own file #204

Closed
wants to merge 3 commits into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jul 9, 2019

This PR moves all unit tests into their own file so that they are not included when pulling the math module via a #[path] import (like the one in compiler builtins, or in a crate that wants to build a cdylib).

@alexcrichton
Copy link
Member

I'm not sure what the reason for this is? I personally think it's great to be able to write tests inline and have them executed. You're in general not really supposed to use #[path] to import crates but even if it were used these are always configured out, right?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 9, 2019

The compiler-builtin crate uses #[path], and since people forget to #[cfg(test)] their tests out (e.g. https://github.com/rust-lang-nursery/libm/blob/de8ed2dbc0fe89e55369cc8eba706b6c3d8086be/src/math/sin.rs#L82), this means that #[test] end up always compiled into compiler-builtins unconditionally.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 9, 2019

Alternatively, we could properly cfg() all tests here, but then we should have a way to enforce that no #[test] end up being generated for compiler builtins.

In the long term, I would prefer these tests to move to the libm-test crate, as proposed in #198 , since they are going to require more tools to actually work (the _eq functions at the top level of the libm crate do not cut it).

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jul 10, 2019

The last commit moves the musl reference tests and required functionality into its own file.

@alexcrichton
Copy link
Member

It may be worthwhile to point out that #[test] implicitly expands to #[cfg(test)]. There is no need to use both #[cfg(test)] and #[test] on a function.

I continue to not think that this change is worth it since it's more readable to have tests inline and I don't think there's a reason to move them out.

@gnzlbg gnzlbg closed this Jul 10, 2019
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.

2 participants