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

Testsuite tests missing recommended ttp:profile attribute #130

Closed
skynavga opened this issue Jan 14, 2016 · 9 comments
Closed

Testsuite tests missing recommended ttp:profile attribute #130

skynavga opened this issue Jan 14, 2016 · 9 comments
Assignees

Comments

@skynavga
Copy link
Contributor

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).

@skynavga skynavga changed the title Test suite TTML files missing recommended ttp:profile attribute Testsuite files missing recommended ttp:profile attribute Jan 14, 2016
@skynavga skynavga changed the title Testsuite files missing recommended ttp:profile attribute Testsuite tests missing recommended ttp:profile attribute Jan 14, 2016
@skynavga skynavga added the bug label Jan 14, 2016
@palemieux palemieux added enhancement and removed bug labels Jan 15, 2016
@palemieux
Copy link
Contributor

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
Copy link
Contributor Author

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
#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
#130 (comment).

@skynavga
Copy link
Contributor Author

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
#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
#130 (comment).

@palemieux
Copy link
Contributor

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:

  • defer this ticket to the next revision of IMSC
  • improve the two example documents included in the body of the specification (as part of a separate issue)

@skynavga
Copy link
Contributor Author

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

@nigelmegitt
Copy link
Contributor

[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
Copy link
Contributor Author

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
#130 (comment).

@skynavga skynavga added the bug label Jan 21, 2016
@skynavga
Copy link
Contributor Author

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
Copy link
Contributor

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.

skynavga added a commit that referenced this issue Jan 28, 2016
skynavga added a commit that referenced this issue Jan 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants