search

apollographql / apollo-federation-subgraph-compatibility

A repo to test subgraph libraries compatibility with the Apollo Federation Specification
https://www.apollographql.com/docs/federation/building-supergraphs/supported-subgraphs/
MIT License
77 stars 58 forks source link

`--failOnRequired`/`--failOnWarning` interact in surprising way #507

Closed myronmarston closed 1 year ago

myronmarston commented 1 year ago

I read the help docs:

$ npx @apollo/federation-subgraph-compatibility@2.0.0 docker --help
Usage: fedtest docker [options]

Start supergraph using Docker Compose

Options:
  --compose <docker-compose.yaml>  Path to docker compose file
  --debug                          debug mode with extra log info
  --failOnRequired                 boolean flag to indicate whether any failing required test should fail the script.
  --failOnWarning                  boolean flag to indicate whether any failing test should fail the script.
  --format <json|markdown>         optional output file format (choices: "json", "markdown", default: "markdown")
  -h, --help                       display help for command
  --path <path>                    GraphQL endpoint path (default: "")
  --port <port>                    HTTP server port (default: "4001")
  --quiet                          suppress logging to minimum
  --schema <schema.graphql>        Path to schema file

...and thought "I want my script to fail on both required tests and for any test", and proceeded to run with --failOnRequired --failOnWarning. However, I discovered that --failOnWarning is ignored in this case.

It looks like the culprit is here:

https://github.com/apollographql/apollo-federation-subgraph-compatibility/blob/e952694dccc80584c18366766e459fc7d86f206f/packages/compatibility/src/compatibilityTest.ts#L61-L67

If --failOnRequired is set, --failOnWarning is silently ignored. Since --failOnWarning is a stricter flag, it seems like it should be checked first. (And I realize now that I don't need to provide both.)

dariuszkuc commented 1 year ago

Thanks for raising this! Indeed the checks should be reversed so it works consistently if both flags are provided. Do you mind opening up a PR with a fix?