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

Fix tests #68

Merged
merged 31 commits into from
Nov 16, 2016
Merged

Fix tests #68

merged 31 commits into from
Nov 16, 2016

Conversation

bergie
Copy link
Contributor

@bergie bergie commented Nov 15, 2016

Seems like our tests were not actually catching invalid schemas. This PR rectifies that.

format: 'date-time'
anyOf: [
'$ref': 'headline.json'
,
Copy link
Contributor

Choose a reason for hiding this comment

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

Use { '$ref': 'headline.json' } and you don't have to have the comma on a separate line. Or at all, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is arguable which syntax is prettier. But anyway, the main point here is to get back to point where we're actually testing things

@@ -21,7 +20,7 @@ describe 'Schemas', ->
schemas.forEach (schema) ->
describe "#{schema.id} (#{schema.title})", ->
try
cases = lib.getExamples schemaName
cases = lib.getExamples path.basename schema.id, path.extname schema.id
Copy link
Contributor

Choose a reason for hiding this comment

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

Why, what is the difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getExamples works on names only (contentblock), while schema.id is full filename (contentblock.json)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but but schemaName (which was used before) is also just the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but we're now in a separate loop

@jonnor
Copy link
Contributor

jonnor commented Nov 15, 2016

Was it only invalid schemas which were not caught? And why were they not caught by checking invalid examples?

@bergie
Copy link
Contributor Author

bergie commented Nov 15, 2016

@jonnor we were not catching them because:

  • JSON meta schema validation was completely wrong (not catching invalid schemas)
  • Because of this we had false positives since previously we were not checking missed

@bergie
Copy link
Contributor Author

bergie commented Nov 16, 2016

@jonnor this iteration should be good to go. Next step will be validating required etc

@jonnor
Copy link
Contributor

jonnor commented Nov 16, 2016

@bergie What do you mean by "validating required"?

@bergie
Copy link
Contributor Author

bergie commented Nov 16, 2016

@jonnor going through them and making only input-side required attributes required so this can be actually used for validating inbound data structures. Now we have some requireds that are only required on responses

@jonnor
Copy link
Contributor

jonnor commented Nov 16, 2016

@bergie Ok. Please put them into a responseRequired key in that case, so we have the information?

@jonnor
Copy link
Contributor

jonnor commented Nov 16, 2016

Or requiredInResponse or whatever

@bergie
Copy link
Contributor Author

bergie commented Nov 16, 2016

@jonnor can we merge? This should fix a lot of stuff already

@jonnor
Copy link
Contributor

jonnor commented Nov 16, 2016

Yep

@jonnor jonnor merged commit 3800825 into master Nov 16, 2016
@jonnor jonnor deleted the fix_tests branch November 16, 2016 14:00
@bergie bergie changed the title WIP: Fix tests Fix tests Nov 16, 2016
@bergie bergie mentioned this pull request Nov 16, 2016
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.

None yet

2 participants