Skip to content

Conversation

mmccool
Copy link
Contributor

@mmccool mmccool commented Mar 4, 2019

In order to generate test results compatible with the change described here, w3c/wot-thing-description#474, it was necessary to update the AssertionTester/Assertion schemas. Unfortunately this was not as simple as just renaming files since some schemas tested keywords in multiple contexts and to be compatible with the new naming scheme these schemas needed to be broken up into simpler schemas that check for each keyword in one specific context.

@mmccool
Copy link
Contributor Author

mmccool commented Mar 4, 2019

Note I got a little confused about the use of "if" and "is-complex" but I think I have it figured out now... at least I know how to get it to give me sane results. Some of my older schema refactorings were wrong though so I have to go back and fix them.

@egekorkan
Copy link
Member

The approval was a mistake, I wanted to approve one file but ended up approving everything

@egekorkan
Copy link
Member

It seems that the files you created are completely correct. However, when you are doing edits, make sure that you change the ids found at:

  • title (which you already do)
  • const, where the intended failure happens (which you forgot sometimes)
  • also key on the root of the document: This basically takes the result of the validation and copies to the ids found at the also key. This is quite tricky so you can leave for the end.

Copy link
Member

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

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

These are correct but you can remove the additional keys that are validated and leave only the format. This way, if a key that is not required for this assertion changes, we will not have to change this schema

Copy link
Member

@egekorkan egekorkan left a comment

Choose a reason for hiding this comment

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

In td-vocab-description--InteractionAffordance.json you can remove the data schema

@mmccool
Copy link
Contributor Author

mmccool commented Mar 5, 2019

Thanks for the feedback... yeah, some of the errors you note were early files I edited before I figured out completely how things worked. I need to go back and update some things to get the right pass/fail/not-impl results. As for "also", probably the only "also" clause I want is for td-vocabulary, to roll up and summarize the vocab results. In general though I think combining results is better done in the implementation report generator once I have results from ALL the implementations in one place.

@mmccool
Copy link
Contributor Author

mmccool commented Mar 5, 2019

I finished my first pass through everything but there are still a few gaps (for example, there is no test for securityDefinitions, and a few other terms) and problems, including inconsistencies with the spec, so it needs another round of review and edits. I also updatd my PR on wot-thing-directory with the test results.
Unfortunately, I will be busy today with other things so... @egekorkan can you take a look, fix any issues you find, and merge when you get a chance? Even if there are problems I would like to update and merge everything (eg test results in the wot repo) before the plugfest call. We can fix any remaining issues over the next week.

@mmccool
Copy link
Contributor Author

mmccool commented Mar 5, 2019

BTW we probably want to also think about how we can restructure this so that changes to the spec don’t create huge editing headaches. For example, if we add “descriptions” and “titles” to SecurityScheme, dozens of files currently would need to be updated. I’m wondering if we can use $refs to a single reference json schema (in a separate file) rather than repeating stuff in each schema. Under the current approach it’s too easy for definitions to get out of sync.

@mmccool mmccool changed the title WIP: Vocab ids with context Vocab ids with context Mar 6, 2019
@egekorkan egekorkan merged commit 1db0319 into eclipse-thingweb:master Mar 7, 2019
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