run round_trip's assertions in separate threads #53

p-alik · 2020-05-24T15:24:43Z

This approach ensures the test reachs killing of client's thread.
This commit aims to solve issue #50

weiznich

I've left some small suggestions for improvements, otherwise this looks fine. Thanks for working on this 👍

weiznich · 2020-05-25T08:42:18Z

wundergraph_cli/src/print_schema/mod.rs


-        child.kill().unwrap();
-        child.wait().unwrap();
+        match handler.join() {


As this is test code it's totally fine to just do a dbg!(handler.join()) here. Cargo does not print output from succeeding tests by default.

So something like dbg!(handler.join()).is_ok() should be fine.

375c03a uses a closure for two Err(..)'s inside request_test. Do you still see demand of amendment?

weiznich · 2020-05-25T08:42:51Z

wundergraph_cli/src/print_schema/mod.rs

@@ -2,6 +2,7 @@ use crate::database::InferConnection;
 use crate::infer_schema_internals::*;
 use std::error::Error;
 use std::io::Write;
+use std::thread;


Seems like this import is not needed?

Indeed, the import is removed in 375c03a

wundergraph_cli/src/print_schema/mod.rs

…ing()

p-alik · 2020-05-26T09:29:25Z

It looks like I've to improve settings of local rustfmt because it produces the style, which fails in CI. Could you provide any advice for proper setup of rustfmt?

weiznich

Generally I'm fine with those changes. I've added some small stylistic suggestions how to improve the implementation.

(Sorry for taking some time to do the review, but real life is currently quite time demanding)

weiznich · 2020-05-27T07:56:52Z

wundergraph_cli/src/print_schema/print_helper.rs

@@ -476,6 +476,16 @@ impl<'a> Display for GraphqlInsertable<'a> {
            let mut out = PadAdapter::new(f);
            writeln!(out)?;
            for c in self.table.column_data.iter().filter(|c| !c.has_default) {
+                let is_pk = self


I think it would be better to add this as part of the filter expression in the line above?

(I'm not sure why this is even missing. I remember writing similar code there 🙈 )

done in 6c70d55

weiznich · 2020-05-28T10:17:57Z

wundergraph_cli/src/print_schema/mod.rs

+        }
+    }
+
+    fn request_test(client: &reqwest::Client, listen_url: &str, body: &'static str) -> bool {


I would prefer to just return a result here and bubble up any potential errors. Then we could just do a t1.is_err() instead of !t1 in the test above. I think this would make the code much clearer.

weiznich · 2020-05-28T10:19:30Z

wundergraph_cli/src/print_schema/mod.rs

+
+        match r {
+            Ok(mut r) => {
+                let builder = std::thread::Builder::new().name("round_trip".into());


It's better to use std::panic::catch_unwind() instead here in my opinion. Also this would allow us to just bubble up the "error" in the fail case.

4152b3d contains std::panic::catch_unwind based approach

This approach ensures the test reachs killing of client's thread.

p-alik · 2020-05-29T09:08:58Z

It looks like I've to improve settings of local rustfmt because it produces the style, which fails in CI. Could you provide any advice for proper setup of rustfmt?

Is max_width defintion in my rustfmt.toml to blame?

$ cat ~/.config/rustfmt/rustfmt.toml 
max_width=80
edition="2018"

an alternative to previous thread based approach

p-alik · 2020-05-29T09:19:55Z

Is max_width defintion in my rustfmt.toml to blame?

It was due to default value

p-alik · 2020-05-29T09:40:14Z

I can't reproduce sqlite test failure locally. I'm still getting the old once for reason Int4 vs Integer.

━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Differences ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: wundergraph_cli/src/print_schema/snapshots/tests__infer_schema.snap
Snapshot: infer_schema
Source: wundergraph_cli/src/print_schema/mod.rs:154
-old snapshot
+new results
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
&s
────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
    2     2 │ use wundergraph::scalar::WundergraphScalarValue;
    3     3 │ use wundergraph::WundergraphEntity;
    4     4 │
    5     5 │ table! {
    6       │-    infer_test.comments (id) {
    7       │-        id -> Int4,
    8       │-        post -> Nullable<Int4>,
    9       │-        commenter -> Nullable<Int4>,
          6 │+    comments (id) {
          7 │+        id -> Integer,
          8 │+        post -> Nullable<Integer>,
          9 │+        commenter -> Nullable<Integer>,
   10    10 │         content -> Text,
   11    11 │     }
   12    12 │ }
   13    13 │

wundergraph_cli/src/print_schema/mod.rs

weiznich · 2020-05-29T13:39:37Z

Thanks for the changes:+1:

can't reproduce sqlite test failure locally. I'm still getting the old once for reason Int4 vs Integer.

I will try to have a look at the weekend/beginning of the next week

It was due to default value

Yes that makes sense. Wundergraph is basically just using the default configs for rustfmt here. I wonder if it could be meaningful to have a rustfmt.toml with default configs in the repo 🤔

Co-authored-by: Georg Semmler <[email protected]>

p-alik

thank you!

+little amemdment in the content

p-alik · 2020-06-04T21:16:34Z

I can't reproduce the new test failure locally.

The only failure I get for
DATABASE_URL=wundergraph_cli.db cargo test --manifest-path wundergraph_cli/Cargo.toml --features sqlite --no-default-features -- --nocapture
is
thread 'print_schema::tests::round_trip' panicked at 'called Result::unwrap()on anErr value: DatabaseError(__Unknown, "database is locked")', wundergraph_cli/src/print_schema/mod.rs:141:21.

But cargo test runs error-free for only round_trip test.

weiznich · 2020-06-04T21:21:51Z

The sqlite backend does not really support running tests in parallel yet, therefore tests need to be run with RUST_THREADS=1 or --test-threads=1 as argument.
If just pushed a commit that should fix the CI, but I'm not sure about that, so let's just wait for the results.

p-alik · 2020-06-05T07:37:10Z

Single threaded cargo test runs error-free for me yet.

weiznich requested changes May 25, 2020

View reviewed changes

replace deprecated std::error::Error::description by Display's to_str…

a008075

…ing()

p-alik force-pushed the issue-50 branch from b26d1e2 to 375c03a Compare May 26, 2020 08:42

p-alik force-pushed the issue-50 branch 2 times, most recently from 29ef069 to 98d7d0d Compare May 26, 2020 18:37

weiznich reviewed May 28, 2020

View reviewed changes

p-alik force-pushed the issue-50 branch from 98d7d0d to 76bd05b Compare May 28, 2020 20:14

Alexei Pastuchov added 3 commits May 28, 2020 23:10

Display implementation of GraphqlInsertable filters out primary keys

6c70d55

the structs New{Comment,Post,User} shouldn't provide id parameter

e738f9d

run round_trip's assertions in separate threads

67feb90

This approach ensures the test reachs killing of client's thread.

p-alik force-pushed the issue-50 branch from 76bd05b to 67feb90 Compare May 28, 2020 21:19

insta assertion is wrapped into std::panic::catch_unwind

178a042

an alternative to previous thread based approach

p-alik force-pushed the issue-50 branch from 4152b3d to 178a042 Compare May 29, 2020 09:18

weiznich reviewed May 29, 2020

View reviewed changes

wundergraph_cli/src/print_schema/mod.rs Outdated Show resolved Hide resolved

p-alik force-pushed the issue-50 branch from 209b49a to 27dda21 Compare May 29, 2020 21:04

Update wundergraph_cli/src/print_schema/mod.rs

eec8b72

Co-authored-by: Georg Semmler <[email protected]>

p-alik force-pushed the issue-50 branch from 27dda21 to eec8b72 Compare May 29, 2020 21:11

weiznich approved these changes Jun 2, 2020

View reviewed changes

Fix infer_schema snapshot for sqlite

55e5531

p-alik commented Jun 4, 2020

View reviewed changes

Alexei Pastuchov and others added 2 commits June 4, 2020 20:41

rename round_trip templates

ee54e46

+little amemdment in the content

Try to fix CI

7cbba1b

Better error messages

a6f16aa

weiznich force-pushed the issue-50 branch from 34f6d4f to a6f16aa Compare June 4, 2020 21:35

p-alik force-pushed the issue-50 branch from c9a1df9 to a6f16aa Compare June 5, 2020 07:52

Cleanup the snapshot mess

ad775a3

weiznich approved these changes Jun 5, 2020

View reviewed changes

weiznich merged commit ffbb883 into weiznich:master Jun 5, 2020

weiznich mentioned this pull request Jun 5, 2020

wundergraph_cli's print_schema::tests::round_trip test fails #50

Closed

p-alik deleted the issue-50 branch June 5, 2020 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

run round_trip's assertions in separate threads #53

run round_trip's assertions in separate threads #53

p-alik commented May 24, 2020

weiznich left a comment

weiznich May 25, 2020

p-alik May 26, 2020

weiznich May 25, 2020

p-alik May 26, 2020

p-alik commented May 26, 2020

weiznich left a comment

weiznich May 27, 2020

p-alik May 29, 2020

weiznich May 28, 2020

p-alik May 29, 2020

weiznich May 28, 2020

p-alik May 29, 2020

p-alik commented May 29, 2020

p-alik commented May 29, 2020

p-alik commented May 29, 2020

weiznich commented May 29, 2020

p-alik left a comment

p-alik commented Jun 4, 2020

weiznich commented Jun 4, 2020

p-alik commented Jun 5, 2020

run round_trip's assertions in separate threads #53

run round_trip's assertions in separate threads #53

Conversation

p-alik commented May 24, 2020

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-alik commented May 26, 2020

weiznich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p-alik commented May 29, 2020

p-alik commented May 29, 2020

p-alik commented May 29, 2020

weiznich commented May 29, 2020

p-alik left a comment

Choose a reason for hiding this comment

p-alik commented Jun 4, 2020

weiznich commented Jun 4, 2020

p-alik commented Jun 5, 2020