search

w3c / imsc

TTML Profiles for Internet Media Subtitles and Captions (IMSC)
https://w3c.github.io/imsc/
Other
31 stars 17 forks source link

Testsuite tests missing recommended ttp:profile attribute #130

Closed skynavga closed 8 years ago

skynavga commented 8 years ago

All of the TTML files in the test suite should specify a ttp:profile attribute to satisfy the recommendation that a ttp:profile attribute SHOULD be specified (if not an EBU-TT-D document).

palemieux commented 8 years ago

I have changed the label to "enhancement" since ttp:profile is not required, merely strongly recommended. As with #133, I would rather not risk changing the test suite at this point of the process unless absolutely required. I suggest discussing this at an upcoming TTWG telecon.

skynavga commented 8 years ago

Please provide a technical reason that this change presents a risk. All test files should respect SHOULD requirements unless their express purpose is to test behavior when the recommendation is not followed.

On Fri, Jan 15, 2016 at 12:11 AM, Pierre-Anthony Lemieux < notifications@github.com> wrote:

I have changed the label to "enhancement" since ttp:profile is not required, merely strongly recommended. As with #133 https://github.com/w3c/imsc/issues/133, I would rather not risk changing the test suite at this point of the process unless absolutely required. I suggest discussing this at an upcoming TTWG telecon.

— Reply to this email directly or view it on GitHub https://github.com/w3c/imsc/issues/130#issuecomment-171887988.

skynavga commented 8 years ago

Also, it seems bad form to write tests that produce warnings in TTV when those warnings could easily be avoided. By default, TTV produces a warning for every violation of a recommendation. While it is possible to disable all or specific warnings in TTV, any review or audit of results should pay extra scrutiny to warnings.

On Fri, Jan 15, 2016 at 1:17 AM, Glenn Adams glenn@skynav.com wrote:

Please provide a technical reason that this change presents a risk. All test files should respect SHOULD requirements unless their express purpose is to test behavior when the recommendation is not followed.

On Fri, Jan 15, 2016 at 12:11 AM, Pierre-Anthony Lemieux < notifications@github.com> wrote:

I have changed the label to "enhancement" since ttp:profile is not required, merely strongly recommended. As with #133 https://github.com/w3c/imsc/issues/133, I would rather not risk changing the test suite at this point of the process unless absolutely required. I suggest discussing this at an upcoming TTWG telecon.

— Reply to this email directly or view it on GitHub https://github.com/w3c/imsc/issues/130#issuecomment-171887988.

palemieux commented 8 years ago

This is really about risk management: any substantive change at this point of the process may have unintended consequences (see recent prohibition of ttp:profile element).

Instead I suggest that we:

skynavga commented 8 years ago

I insist on fixing in IMSC1. We can discuss in upcoming telecons.

nigelmegitt commented 8 years ago

[not facetious]: isn't it a good thing that you can verify that TTV generates a warning when a recommendation is not followed?

Modifying the test suite at this late stage where there is no actual conformance issue carries the risk that existing implementations that currently pass the test fail it after the change has been made. Clearly that would be an indication that the implementation is not behaving correctly, but that's not our primary concern right now from the perspective of moving to Proposed Recommendation. To reduce this risk can we address this by:

  1. Adding the option in the expected test results that validating processors are permitted to issue a warning due to the recommendation not being followed. This would not affect the status of any existing implementation that has passed the test, hence mitigating the issue.
  2. Add further derived tests in a 'not required for advancing to PR' set where the only change is that the recommendation is followed correctly, with a test result description that the output should be identical to the originating test, but that correctly functioning validating processor implementations will not issue a recommendation-not-followed warning.

Would that be an acceptable way forward? Let's discuss in today's meeting if possible.

skynavga commented 8 years ago

I can only agree to a solution that entails fixing all warnings in the test suite, except and only for tests whose purpose is to verify that a warning is produced.

The only risk in not doing this change is that some existing implementation will somehow fail due to the presence of these optional features. However, if that is the case, then that is a non compliant implementation and we should not allow such an implementation to pass the tests.

On Thu, Jan 21, 2016 at 4:48 AM, nigelmegitt notifications@github.com wrote:

[not facetious]: isn't it a good thing that you can verify that TTV generates a warning when a recommendation is not followed?

Modifying the test suite at this late stage where there is no actual conformance issue carries the risk that existing implementations that currently pass the test fail it after the change has been made. Clearly that would be an indication that the implementation is not behaving correctly, but that's not our primary concern right now from the perspective of moving to Proposed Recommendation. To reduce this risk can we address this by:

  1. Adding the option in the expected test results that validating processors are permitted to issue a warning due to the recommendation not being followed. This would not affect the status of any existing implementation that has passed the test, hence mitigating the issue.
  2. Add further derived tests in a 'not required for advancing to PR' set where the only change is that the recommendation is followed correctly, with a test result description that the output should be identical to the originating test, but that correctly functioning validating processor implementations will not issue a recommendation-not-followed warning.

Would that be an acceptable way forward? Let's discuss in today's meeting if possible.

— Reply to this email directly or view it on GitHub https://github.com/w3c/imsc/issues/130#issuecomment-173547212.

skynavga commented 8 years ago

I restored the bug label because I believe it is a bug to have a test produce a warning unless it is the express intent of the test to verify the production of the warning.

nigelmegitt commented 8 years ago

Notes from today's TTWG meeting: the view is to update all tests so that they do not generate warnings from processors that check for conformance with specification recommendations, unless the specific goal of the test is to check that such warnings are issued when they apply.