Improving code coverage

[max-sixty]

What's up?

One Good First Issue for folks who want to start writing rust — now we have Code Coverage — is to find some code that isn't tested, and write a small test for it.

Our tests are really easy to write, and by mapping a test to the code that gets run, it'll give a good intro to the compiler.

[max-sixty]

Edit: this specific example is done! Thanks to @nkicg6 in #3985.

I'm leaving the broader issue open since there's lots more out there...

---

As an example of the issue, replacing Span's Serialize & Deserialize with standard implementations (diff below) doesn't break tests, so we don't know if this code is doing what's intended.

I found that by browsing the crates path in codecov.io, and finding a file with very low coverage: https://app.codecov.io/github/prql/prql/commit/881e03fcc45bd7bd1859e864c55358bf68c652e0/blob/crates/prql-ast/src/span.rs.

diff --git a/crates/prql-ast/src/span.rs b/crates/prql-ast/src/span.rs
index 97c8319c..cd8b215e 100644
--- a/crates/prql-ast/src/span.rs
+++ b/crates/prql-ast/src/span.rs
@@ -3,7 +3,7 @@
 use std::fmt::{self, Debug, Formatter};
 use std::ops::Range;
 
-#[derive(Clone, PartialEq, Eq, Copy)]
+#[derive(Clone, PartialEq, Eq, Copy, Serialize, Deserialize)]
 pub struct Span {
     pub start: usize,
     pub end: usize,
@@ -22,69 +22,3 @@ fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
         write!(f, "{}:{}-{}", self.source_id, self.start, self.end)
     }
 }
-
-impl Serialize for Span {
-    fn serialize<S>(&self, serializer: S) -> std::result::Result<S::Ok, S::Error>
-    where
-        S: serde::Serializer,
-    {
-        let str = format!("{self:?}");
-        serializer.serialize_str(&str)
-    }
-}
-
-impl<'de> Deserialize<'de> for Span {
-    fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
-    where
-        D: serde::Deserializer<'de>,
-    {
-        struct SpanVisitor {}
-
-        impl<'de> Visitor<'de> for SpanVisitor {
-            type Value = Span;
-
-            fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result {
-                write!(f, "A span string of form `file_id:x-y`")
-            }
-
-            fn visit_str<E>(self, v: &str) -> std::result::Result<Self::Value, E>
-            where
-                E: serde::de::Error,
-            {
-                use serde::de;
-
-                if let Some((file_id, char_span)) = v.split_once(':') {
-                    let file_id = file_id
-                        .parse::<u16>()
-                        .map_err(|e| de::Error::custom(e.to_string()))?;
-
-                    if let Some((start, end)) = char_span.split_once('-') {
-                        let start = start
-                            .parse::<usize>()
-                            .map_err(|e| de::Error::custom(e.to_string()))?;
-                        let end = end
-                            .parse::<usize>()
-                            .map_err(|e| de::Error::custom(e.to_string()))?;
-
-                        return Ok(Span {
-                            start,
-                            end,
-                            source_id: file_id,
-                        });
-                    }
-                }
-
-                Err(de::Error::custom("malformed span"))
-            }
-
-            fn visit_string<E>(self, v: String) -> std::result::Result<Self::Value, E>
-            where
-                E: serde::de::Error,
-            {
-                self.visit_str(&v)
-            }
-        }
-
-        deserializer.deserialize_string(SpanVisitor {})
-    }
-}

So adding a test which does .to_string() on these & back would be great!

[nkicg6]

I'll keep chipping away at this, like you said it is a good way to get the hang of the compiler!

[max-sixty]

I'll keep chipping away at this, like you said it is a good way to get the hang of the compiler!

Great!

And if there are questions like "what would be a good way of testing this code" or "why doesn't this code get run given this test", that's very welcome. (I've sometimes asked this myself, @aljazerzen has responded, and I've then pasted it as a comment, which can be a nice way of helping make the compiler more legible...)

And FYI I adjusted the link above slightly, to focus on the compiler — the coverage for the CLI isn't accurate atm.

[max-sixty]

I just merged #3987, which now includes the prqlc binary tests as an input to coverage. So now we have a more accurate picture of what's covered...

[nkicg6]

I'm curious about the diff checks in the compiler integration tests that I ran into it when I wrote a quick unit test in the prqlc-ast package. expr.rs and stmt.rs both have // The following code is tested by the tests_misc crate..., which looks like it is being used as a marker to split the code for the diff in the integration test (I see tests_misc was moved to prql-compiler/src/tests/ around here). My main question is around test architecture-- should I focus on integration tests for the compiler (prqlc-compiler) and not unit testing things in prqlc-ast (also given that prqlc-ast has test = false)?

Is there a write up or thread I can read to better understand why expr.rs and stmt.rs exist with shared fields/variants in two places?

[max-sixty]

prql-ast looks fairly well covered, but definitely still holes. For sure feel free to remove test = false if we can add tests there (it's not a policy, we just didn't have any tests yet!)

https://app.codecov.io/github/prql/prql/tree/main/prqlc%2Fprqlc-ast.

Yes, good questions on the repeated code. Others have more detail than me, but mostly it's because we wanted a single crate with an externally facing AST. It was the result of a big refactor. Arguably we haven't done that much with it, so we're holding some costs of the additional abstraction at the moment. (I had a look for some of the discussion, but couldn't immediately find it, sorry to be brief)