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

Add JSON schema documenting logs #69

Merged
merged 12 commits into from
Sep 2, 2021

Conversation

abailly-iohk
Copy link
Contributor

This adds the schema file and the related conformance tests adapted from APISpec to check generated JSON conforms with the specification.

NOTE: Not all log entries are documented (yet)

@github-actions
Copy link

github-actions bot commented Aug 30, 2021

Unit Test Results

    4 files  ±0    45 suites  ±0   1m 27s ⏱️ ±0s
134 tests ±0  131 ✔️ ±0  3 💤 ±0  0 ❌ ±0 

Results for commit f72ef52. ± Comparison against base commit f72ef52.

♻️ This comment has been updated with latest results.

@abailly-iohk abailly-iohk force-pushed the abailly-iohk/test-conformance-of-logs branch from 0f096ed to 2791ac8 Compare September 1, 2021 13:48
@abailly-iohk
Copy link
Contributor Author

@KtorZ I think I have addressed your concerns about the holes in checking specs.

Copy link
Collaborator

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

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

Had another round, although you not explicitly re-requested review :)

README.md Outdated Show resolved Hide resolved
hydra-node/api-log.yaml Outdated Show resolved Hide resolved
hydra-node/api-log.yaml Outdated Show resolved Hide resolved
hydra-node/api-log.yaml Outdated Show resolved Hide resolved
hydra-node/api-log.yaml Outdated Show resolved Hide resolved
Text ->
FilePath ->
Property
prop_validateToJSON specFile namespace inputFile =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could: The inputFile is redundant (and complicates the interface of having two filepaths). Instead the function could require Typeable and use typeOf to create a filename unique per-type to test in a temporary directory.

, -- This second sub-property ensures that any key found in the
-- specification corresponds to a constructor in the corresponding
-- data-type. This in order the document in sync and make sure we don't
-- left behind constructors which no longer exists.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this also apply to the log api? If so, the given namespace "logs" is nowhere found in the api-logs.yaml and I wonder whether this works.

hydra-node/src/Hydra/Chain.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/JSONSchema.hs Outdated Show resolved Hide resolved
hydra-node/test/Hydra/JSONSchema.hs Outdated Show resolved Hide resolved
abailly-iohk and others added 11 commits September 2, 2021 13:54
This will need specific tests for roundtripping, stability of JSON
format and conformance with the published schema
This extracts and generalises code previously from the APISpec module
to be used by LoggingSpec module in order to test conformance of log
entries with JSON schema.
Added log entries for Events
Also provide fields for the PostChainTx constructors
The completeness property is very dependent on some specific structure
of the specification YAML and not suited for all types which are
specified in different places in the Yaml
Also addresses some review comments:
* Unify punctuation in yaml spec
* Move Envelope to Logging module and use it
* Add more comments to prop_specIsComplete
@abailly-iohk abailly-iohk force-pushed the abailly-iohk/test-conformance-of-logs branch from fa97b09 to e729aef Compare September 2, 2021 13:55

-- NOTE(AB): This is a bit contrived but we want a numeric threadId and we
-- get some text which we know the structure of
mkThreadId = fromMaybe 0 . readMaybe . Text.unpack . Text.drop 9 . show
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's stopping us from having a threadId :: Text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The schema would not fit, so we would need to change both. I agree this expression is annoying hence my comment.

-- - title: APIServer
-- type: object
-- ...
-- @@
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

]

apiSpecificationSelector :: SpecificationSelector
apiSpecificationSelector = key "properties" . key "message"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could: inline as it's short enough now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do it in master

@abailly-iohk abailly-iohk merged commit f72ef52 into master Sep 2, 2021
@abailly-iohk abailly-iohk deleted the abailly-iohk/test-conformance-of-logs branch September 2, 2021 14:50
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.

3 participants