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

Adds flag to mark some tests as out-of-scope for ion-schema-schemas #35

Merged
merged 1 commit into from
Oct 5, 2022

Conversation

popematt
Copy link
Contributor

@popematt popematt commented Oct 3, 2022

Issue #, if available:

#32

Description of changes:

Updates the test spec with a way to mark tests that should be skipped when testing with ISL for ISL, and updates all Ion Schema Tests that need to be skipped.

Rather than having a blocked/skip list in every implementation that runs the ISL for ISL tests, we should just mark the test cases themselves and include a reason why the test case is to be skipped. This approach is better than the existing approach because it is more fine-grained than having a file-based blocked/skip list, and it documents the reason the particular test cases are skipped.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link

@tgregg tgregg left a comment

Choose a reason for hiding this comment

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

I appreciate the idea behind documenting the reason for the skip, but in reality the reason is always because ISL for ISL can't validate <description>. To avoid the repetitiveness, I'd be okay with something like isl_does_not_validate: true.

}

$test::{
description: "byte_length range must not be non-empty",
Copy link

Choose a reason for hiding this comment

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

The phrase "must not be non-empty" occurs in many of these, and it really confused me until I realized that "empty" referred to the values that could satisfy the range, rather than the range declaration itself. But even if "empty" weren't confusing, don't we mean "must not be empty"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right—there's an accidental double negative there.

"range must be satisfiable" maybe?

@popematt
Copy link
Contributor Author

popematt commented Oct 4, 2022

I appreciate the idea behind documenting the reason for the skip, but in reality the reason is always because ISL for ISL can't validate <description>. To avoid the repetitiveness, I'd be okay with something like isl_does_not_validate: true.

I understand where you're coming from, but I guess I don't mind the repetitiveness compared to the value from being able to come back to it later and read why it is that ISL for ISL cannot validate (or invalidate?) it.

I don't hold this opinion very strongly, though, so I will switch to something without the string value.

README.md Outdated Show resolved Hide resolved
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