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

Cant declare inline tests with dependencies #202

Closed
GregBowyer opened this issue Mar 7, 2019 · 4 comments · Fixed by #203
Closed

Cant declare inline tests with dependencies #202

GregBowyer opened this issue Mar 7, 2019 · 4 comments · Fixed by #203

Comments

@GregBowyer
Copy link
Contributor

Sorry if this is already raised

Consider a file like so

fn some_fn() -> u32 {
    2
}

#[cfg(test)]
mod tests {
    use super::some_fn();
    
    // This following line is not going to work
    use pretty_assertions::assert_eq;
    
    #[test]
    fn test_is_2() {
        assert_eq!(1, some_fn());
    }
}

With a build function like so

rust_library(
    name = "example",
    srcs = ["lib.rs"]
)

rust_test(
    name = "example_test",
    deps = [
        ":example",
        "//third_party/rust:pretty_assertion",
    ],
)

You will get an error along the lines of

attribute srcs: No lib.rs or example_test.rs source file found.

Is there something I am missing?

@mfarrugi
Copy link
Collaborator

mfarrugi commented Mar 8, 2019 via email

GregBowyer added a commit to GregBowyer/rules_rust that referenced this issue Mar 8, 2019
Due to the way unit tests are handled with the rules, it is tricky for a
unit test to have additional deps.

Consider some source code like such

```rust
fn some_fn() -> u32 {
    2
}

mod tests {
    use super::some_fn();

    // This following line is not going to work
    use pretty_assertions::assert_eq;

    #[test]
    fn test_is_2() {
        assert_eq!(1, some_fn());
    }
}
```

With a build function like so

```python
rust_library(
    name = "example",
    srcs = ["lib.rs"]
)

rust_test(
    name = "example_test",
    deps = [
        ":example",
        "//third_party/rust:pretty_assertion",
    ],
)
```

Presently, due to the handling of ‟unit tests” the extra deps will
conflict with rust_rules ideas about how to identify the test, and give
an error along the lines of

```
attribute srcs: No lib.rs or example_test.rs source file found.
```

With this change we make it possible for tests to express ‟dev
dependencies”. This lets us avoid adding deps that are strictly for
testing.

This is somewhat distinct from `testonly` attributes as it does not
always hold that a dependency is always and only ever `testonly` in all
library circumstances, consider for example the usage of
https://github.com/pingcap/fail-rs. There are instances where this needs
to be used both in source code *and* test code, and is not easily
seperated from a cargo-raze perspective.

Closes bazelbuild#202
@GregBowyer
Copy link
Contributor Author

@mfarrugi I think there are two possible solutions.

One is to allow tests to maybe have a dev_deps attribute (see #203).
The other one might be to fiddle about with the notion of test_only deps (similar to the cc_library rule does).

I think the first is a little more idiomatic to how Rust / Cargo works and is possibly easier for those coming from Cargo to understand, albeit at the cost of being a little none-typical for looking like idiomatic Bazel. With that said language rules can naturally have extensions that are specialised (for instance crate_type) so its not impossible to take such an approach.

Thoughts ? ideas?

@mfarrugi
Copy link
Collaborator

mfarrugi commented Mar 8, 2019 via email

@bspeice
Copy link

bspeice commented Mar 24, 2019

Just got bit by this - for what it's worth, I'd vote having a dev-deps in the rust_library or rust_binary rules since the Cargo [dev-dependencies] is also intended for use with examples and benchmarks. Happy to take a crack at it if someone can point me where to get started.

GregBowyer added a commit to GregBowyer/rules_rust that referenced this issue Apr 4, 2019
Due to the way unit tests are handled with the rules, it is tricky for
a crate containing internal test configuration to have additional (test
only) deps.

Consider some source code like such

```rust
fn some_fn() -> u32 {
    2
}

mod tests {
    use super::some_fn();

    // This following line is not going to work
    use pretty_assertions::assert_eq;

    #[test]
    fn test_is_2() {
        assert_eq!(1, some_fn());
    }
}
```

With a build function like so

```python
rust_library(
    name = "example",
    srcs = ["lib.rs"]
)

rust_test(
    name = "example_test",
    deps = [
        ":example",
        "//third_party/rust:pretty_assertion",
    ],
)
```

Presently, due to the handling of ‟unit tests” the extra deps will
conflict with rust_rules ideas about how to identify the test, and give
an error along the lines of

```
attribute srcs: No lib.rs or example_test.rs source file found.
```

With this change we make it possible for tests to express ‟dev
dependencies”. This lets us avoid adding deps that are strictly for
testing.

This is somewhat distinct from `testonly` attributes as it does not
always hold that a dependency is always and only ever `testonly` in all
library circumstances, consider for example the usage of
https://github.com/pingcap/fail-rs. There are instances where this needs
to be used both in source code *and* test code, and is not easily
seperated from a cargo-raze perspective.

Closes bazelbuild#202

WIP
GregBowyer added a commit to GregBowyer/rules_rust that referenced this issue Apr 4, 2019
Due to the way unit tests are handled with the rules, it is tricky for
a crate containing internal test configuration to have additional (test
only) deps.

Consider some source code like such

```rust
fn some_fn() -> u32 {
    2
}

mod tests {
    use super::some_fn();

    // This following line is not going to work
    use pretty_assertions::assert_eq;

    #[test]
    fn test_is_2() {
        assert_eq!(1, some_fn());
    }
}
```

With a build function like so

```python
rust_library(
    name = "example",
    srcs = ["lib.rs"]
)

rust_test(
    name = "example_test",
    deps = [
        ":example",
        "//third_party/rust:pretty_assertion",
    ],
)
```

Presently, due to the handling of ‟unit tests” the extra deps will
conflict with rust_rules ideas about how to identify the test, and give
an error along the lines of

```
attribute srcs: No lib.rs or example_test.rs source file found.
```

With this change we make it possible for tests to express ‟dev
dependencies”. This lets us avoid adding deps that are strictly for
testing.

This is somewhat distinct from `testonly` attributes as it does not
always hold that a dependency is always and only ever `testonly` in all
library circumstances, consider for example the usage of
https://github.com/pingcap/fail-rs. There are instances where this needs
to be used both in source code *and* test code, and is not easily
seperated from a cargo-raze perspective.

Closes bazelbuild#202

WIP
GregBowyer added a commit to GregBowyer/rules_rust that referenced this issue Apr 4, 2019
Due to the way unit tests are handled with the rules, it is tricky for
a crate containing internal test configuration to have additional (test
only) deps.

Consider some source code like such

```rust
fn some_fn() -> u32 {
    2
}

mod tests {
    use super::some_fn();

    // This following line is not going to work
    use pretty_assertions::assert_eq;

    #[test]
    fn test_is_2() {
        assert_eq!(1, some_fn());
    }
}
```

With a build function like so

```python
rust_library(
    name = "example",
    srcs = ["lib.rs"]
)

rust_test(
    name = "example_test",
    deps = [
        ":example",
        "//third_party/rust:pretty_assertion",
    ],
)
```

Presently, due to the handling of ‟unit tests” the extra deps will
conflict with rust_rules ideas about how to identify the test, and give
an error along the lines of

```
attribute srcs: No lib.rs or example_test.rs source file found.
```

With this change we make it possible for tests to express ‟dev
dependencies”. This lets us avoid adding deps that are strictly for
testing.

This is somewhat distinct from `testonly` attributes as it does not
always hold that a dependency is always and only ever `testonly` in all
library circumstances, consider for example the usage of
https://github.com/pingcap/fail-rs. There are instances where this needs
to be used both in source code *and* test code, and is not easily
seperated from a cargo-raze perspective.

Closes bazelbuild#202

WIP
GregBowyer added a commit to GregBowyer/rules_rust that referenced this issue Apr 5, 2019
Due to the way unit tests are handled with the rules, it is tricky for
a crate containing internal test configuration to have additional (test
only) deps.

Consider some source code like such

```rust
fn some_fn() -> u32 {
    2
}

mod tests {
    use super::some_fn();

    // This following line is not going to work
    use pretty_assertions::assert_eq;

    #[test]
    fn test_is_2() {
        assert_eq!(1, some_fn());
    }
}
```

With a build function like so

```python
rust_library(
    name = "example",
    srcs = ["lib.rs"]
)

rust_test(
    name = "example_test",
    deps = [
        ":example",
        "//third_party/rust:pretty_assertion",
    ],
)
```

Presently, due to the handling of ‟unit tests” the extra deps will
conflict with rust_rules ideas about how to identify the test, and give
an error along the lines of

```
attribute srcs: No lib.rs or example_test.rs source file found.
```

With this change we make it possible for tests to express ‟dev
dependencies”. This lets us avoid adding deps that are strictly for
testing.

This is somewhat distinct from `testonly` attributes as it does not
always hold that a dependency is always and only ever `testonly` in all
library circumstances, consider for example the usage of
https://github.com/pingcap/fail-rs. There are instances where this needs
to be used both in source code *and* test code, and is not easily
seperated from a cargo-raze perspective.

Closes bazelbuild#202

WIP
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 a pull request may close this issue.

3 participants