Conformance Test Suite #237
Conversation
|
|
@ThisIsMissEm hey! I gathered my thoughts after our meeting, and wrote what I think would be an appropriate README for the conformance tests. Let me know if you have any feedback! A note: I thought |
ThisIsMissEm
left a comment
ThisIsMissEm
left a comment
There was a problem hiding this comment.
A fair few comments
|
@StevenLangbroek I think for now we should probably just go with a docker release strategy, instead of npx. I'm not too keen on how you need to setup trusted publishing, so not publishing to npm seems safest. |
|
Still fighting |
- Update license to AGPL-3.0, per reference server
Co-authored-by: Emelia Smith <ThisIsMissEm@users.noreply.github.com> Signed-off-by: Steven Langbroek <steven.android@gmail.com>
- Remove npm mention until publishing has been figured out by maintainers - Rename category to suite - Remove mention of unnecessary test suites
- Add basic nodeinfo test
StevenLangbroek
force-pushed
the
feat/conformance
branch
from
8187eba to
bf42c43
Compare
| "@optique/run": "^0.7.0", | ||
| "bcp-47": "^2.1.0", | ||
| "jsonld": "^9.0.0", | ||
| "ts-node-maintained": "^10.9.6", |
There was a problem hiding this comment.
This should probably be a dev dep
There was a problem hiding this comment.
Should it? It's a dependency for running the tool, no, through cli.js?
There was a problem hiding this comment.
nope, as the distribution would be compiled.
| expect(dataset).toHaveProperty('id'); | ||
| expect(typeof dataset.id).toBe('string'); | ||
|
|
||
| // name is required |
There was a problem hiding this comment.
| // name is required | |
| // name is required | |
| // TODO: support i18n |
There was a problem hiding this comment.
@ThisIsMissEm which properties support this behavior (i18n, field -> fieldMap) again?
There was a problem hiding this comment.
name->nameMapcontent->contentMapsummary->summaryMap
ThisIsMissEm
left a comment
ThisIsMissEm
left a comment
There was a problem hiding this comment.
Looking good, I left a few comments
| }, | ||
| }); | ||
|
|
||
| const data: any = await response.json(); |
There was a problem hiding this comment.
We can dry the above up a bit later
@StevenLangbroek Hey, I'm the author of Optique. Thanks for trying it out! You're right that this pattern is tricky to express with the current API. The I've opened an issue to track a potential solution: dahlia/optique#49 The idea is a const parser = conditional(
option("--reporter", choice(["console", "junit", "html", "json"])),
{
console: object({}),
junit: object({ outputFile: option("--output-file", string()) }),
html: object({ outputFile: option("--output-file", string()) }),
json: object({ outputFile: option("--output-file", string()) }),
},
object({}) // default when --reporter is omitted
);This would keep all reporter values in a single This is high on my priority list, so I'm hoping to get it implemented soon. Feel free to chime in on the issue if you have thoughts on the API design! |
|
@StevenLangbroek The |
|
@dahlia hey! awesome, thanks so much! i'll get to work and let you know how it goes. |
|
@dahlia works a treat, thanks! small note about the API, the syntax for default behavior doesn't intuitively make sense for my use-case. I'd have expected the default to be passed as a string / "choice", e.g.: const parser = conditional(
option("--reporter", choice(["console", "junit", "html", "json"])),
{
console: object({}),
junit: object({ outputFile: option("--output-file", string()) }),
html: object({ outputFile: option("--output-file", string()) }),
json: object({ outputFile: option("--output-file", string()) }),
},
'console' // this makes more sense to me than `object({})`
);But I understand there might be reasons / constraints driving that. I also don't feel super strongly about it, just a thought. |
Hey! Opening Draft PR early to have a place to discuss direction early & continuously. Feel free to offer feedback at any point in time, I'll actively sollicit at key moments myself.
To do