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

Update spec for test changes, add "bench" subcommand #112

Merged
merged 9 commits into from
Mar 4, 2024

Conversation

paf31
Copy link
Collaborator

@paf31 paf31 commented Feb 27, 2024

Updates ndc-spec and adds a bench subcommand for simple benchmarks of connectors built with the SDK:

Usage: ndc_hub_example bench [OPTIONS] --configuration <DIRECTORY> --snapshots-dir <DIRECTORY>

Options:
      --configuration <DIRECTORY>  [env: HASURA_CONFIGURATION_DIRECTORY=]
      --samples <COUNT>            the number of samples to collect per test [default: 100]
      --tolerance <TOLERANCE>      tolerable deviation from previous report, in standard deviations from the mean
      --snapshots-dir <DIRECTORY>  [env: HASURA_SNAPSHOTS_DIR=]
  -h, --help                       Print help

@paf31 paf31 requested a review from SamirTalwar February 27, 2024 22:23
@paf31 paf31 changed the title Draft: update spec dependency for test changes Update spec for test changes, add "bench" subcommand Mar 1, 2024
@paf31 paf31 marked this pull request as ready for review March 1, 2024 18:38
@paf31 paf31 force-pushed the update-spec-for-test branch from 6777068 to cf99c15 Compare March 1, 2024 19:02
ndc-client = { git = "http://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.18" }
ndc-test = { git = "http://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.18" }
ndc-client = { git = "http://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.19" }
ndc-test = { git = "http://github.com/hasura/ndc-spec.git", tag = "v0.1.0-rc.19" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that in the future, we should consider moving the ndc-test-related stuff to a separate, optional crate.

I want this testing and benchmarking but I don't want to ship it to prod.

FWIW we don't use the test command in ndc-postgres; we wrap the test runner in an integration test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could put it behind a feature flag.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

% cargo run
Usage: ndc_hub_example <COMMAND>

Commands:
  serve         
  check-health  
  help          Print this message or the help of the given subcommand(s)

% cargo run --features ndc-test

Usage: ndc_hub_example <COMMAND>

Commands:
  serve         
  test          
  replay        
  bench         
  check-health  
  help          Print this message or the help of the given subcommand(s)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I was thinking, but I'm fine with doing it later.

Cargo.lock Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love to see it.

println!();
println!("{}", report(results));
println!("{}", reporter.1.report());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this necessary? Doesn't the ConsoleReporter handle this now?

(Same applies to the replay function.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reporter only responds to the traversal of the nodes of the test tree structure, it doesn't have a hook for when the tests are fully complete. ConsoleReporter writes the tree out to stdout without allocating, but TestResults collects up the failures. Another approach would be to add a finish method to the trait, but as it is, TestResults is useful for cases where you don't want to write to stdout at all, or immediately.

@paf31 paf31 force-pushed the update-spec-for-test branch from ff2bd89 to b5f3c83 Compare March 1, 2024 21:02
@SamirTalwar SamirTalwar force-pushed the update-spec-for-test branch from a0ba01b to 5554a1b Compare March 4, 2024 08:50
Copy link
Contributor

@SamirTalwar SamirTalwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ship this!

@SamirTalwar SamirTalwar merged commit ec03381 into main Mar 4, 2024
5 checks passed
@SamirTalwar SamirTalwar deleted the update-spec-for-test branch March 4, 2024 09:20
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