-
Notifications
You must be signed in to change notification settings - Fork 20
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
More MO tests #117
More MO tests #117
Conversation
@marisademeglio I pre-empt @dlazin's reactions: you should restructure the file organization of the tests internally, following the templates... (the content should be in an OPS directory...) |
Why are you using OPS, out of curiosity? That's the name of the EPUB 2 structure spec. |
I do not remember... but all our tests use the same structure, which is helpful. Cc @dlazin |
Sure, but that's why you'll find /EPUB directories used in all the post EPUB 2 stuff we did in IDPF. You used to also find a lot of /OEBPS directories, too, which came from the even older Open EBook Publication Structure spec name. It's kind of weird to use EPUB 2 terminology with EPUB 3. |
I have used "EPUB" because I like it better :) I didn't know about the requirement to use "OPS". Do you want me to change it? |
I let @dlazin a.k.a. the boss decide it, but I think it would be better. |
This came from the templates that Dave created; I just cleaned them up for consistency, and wasn't aware that OPS was an EPUB 2 convention; I thought the name was arbitrary and OPS was one of the common choices. (InDesign produces an OEBPS folder, as I'm sure you know.) We can change it, but I would like to be consistent at any point in time, because that way you can more easily tell what the intentional choices in a given test are relative to the default non-choices. So for now, indeed, please do OPS and otherwise match the template in https://github.com/w3c/epub-tests/tree/main/tests/xx-epub-template, and we can rename all tests to EPUB in a separate PR if you think it's worth doing. |
Folder naming aside, Ivan and Marisa mentioned in today's testing call that they both wrote the same test. I have reviewed #118 and am working on #116 right now; does this still conflict? If so, please fix before I review; otherwise let me know and I will try to review tomorrow! Thanks and sorry for the collision. |
Looking at the tests, it looks like one of the tests in the PR ( The other test is in navigation that do not overlap with neither #116 nor #118 in my view. |
I could argue it either way; I was following the example of the tests in the previous MO section (timing and sync), which combine several MUSTs into a single test. The test in "rendering audio" can be for the concept of "media clipping", which is what this PR does; or the tests can be for each permutation of media clipping, as in #116. |
Sure, you can call the folder whatever you want, or not even bother with one. But for a formal test suite, it's a weird association with an acronym of the past. We're not testing OPS or OEBPS structure. That's why we opted to use "EPUB" as the name for all things 3.0+ -- to break the cycle of revision-based acronyms. It's why the specs are all "EPUB XYZ" now, too, as it didn't help to have a format called EPUB defined by standards with acronyms like OPS and OPF. I can fix this for you if you want, though, since I'm the one complaining (in a separate PR, of course). |
Sure, that would be very welcome! I would happily accept a separate PR that renames all OPS folders to EPUB. |
I think that is really the issue: I am afraid this boat has sailed. There are about 100 tests in the repo; we should come up with a script that makes the folder name change in all folders; modify the necessary files within the folders; and re-generate the epub files. Doable of course, but I am not sure it is worth at this point... |
True enough. My pragmatic point is that having more tests does not harm; I would just take all of these. I hate throwing out things that we already have. @dlazin ? |
Oops, I just see that @mattgarrish has done this in #119! I guess this means I would have to change things manually in #116 and #118 |
Thinking about this further... I convinced myself that @marisademeglio is right :-) I will then remove two tests from #116: namely @dlazin your turn in reviewing this PR... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
epubcheck (the version for EPUB 3.3) reports errors on both files:
8:59 tests> epubcheck5 mol-navigation.epub
Validating using EPUB version 3.3 rules.
ERROR(MED_014): mol-navigation.epub/EPUB/mo/ch1.smil(2,43): A non-empty fragment identifier is required.
ERROR(MED_014): mol-navigation.epub/EPUB/mo/ch2.smil(2,37): A non-empty fragment identifier is required.
ERROR(RSC-007): mol-navigation.epub/EPUB/mo/ch1.smil(2,43): Referenced resource "EPUB/xhtml/ch1.xhtml" could not be found in the EPUB.
Check finished with errors
Messages: 0 fatals / 3 errors / 0 warnings / 0 infos
EPUBCheck completed
and
9:07 tests> epubcheck5 mol-navigation.epub
Validating using EPUB version 3.3 rules.
ERROR(MED_014): mol-navigation.epub/EPUB/mo/ch1.smil(2,37): A non-empty fragment identifier is required.
ERROR(MED_014): mol-navigation.epub/EPUB/mo/ch2.smil(2,37): A non-empty fragment identifier is required.
Check finished with errors
Messages: 0 fatals / 2 errors / 1 warning / 0 infos
EPUBCheck completed
The missing file reference is easy (and I submitted an editorial change); for the others the issue is that the reference in the smil file's <body>
element should refer to a fragment in the target xhtml file: ie, and id
value should be given to, e.g., the <body>
element of the relevant xhtml file and referred to from the smil file. (I did those changes on my local machine and epubcheck was happy).
@@ -0,0 +1,20 @@ | |||
<smil xmlns:epub="http://www.idpf.org/2007/ops" xmlns="http://www.w3.org/ns/SMIL" version="3.0"> | |||
<body epub:textref="../xhtml/ch1.xhtml"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<body epub:textref="../xhtml/ch1.xhtml"> | |
<body epub:textref="../ch1.xhtml"> |
FWIW, the navigation test runs on the web version of Colibrio as well as on Thorium on the Mac. The rendering audio one runs on Colibrio but the third 'subtest' ( The exceeding |
Thanks for the file reference fix :) I was not getting EPUBCheck errors with version 4.2.4 but I've now upgraded to v5 and I see what you see. However, the error about there being no fragment identifier in the URL, e.g. " It does not affect this test but does it seem wrong to you too? I can open an issue over in the appropriate repo. Anyway, I am pushing a fix to make EPUBCheck happier, since it doesn't really matter for our purposes here if it's flagging an error when there is none. |
Reviewing now. Just to confirm, we've decided that all the tests in this PR will survive, because Ivan removed some from #116, right? Cool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for the tests! Sorry for the delay and all the comments; most of these are just first-time "here is how we do things" changes, not substantive; next time should be much cleaner.
@charset "utf-8"; | ||
|
||
/*for the active text element*/ | ||
.-epub-media-overlay-active { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: so that our tests have consistent setup, can this be .active-item like in https://github.com/w3c/epub-tests/blob/main/tests/mol-css/EPUB/package.opf?
(Consistency is important so that, when analyzing, we don't have to puzzle out whether something is intentionally different or just arbitrarily so. IMO only things under test should vary from one test to another — hence the EPUB vs OPS debate too.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok we can make it match across all the tests but I don't like the classnames in the test you linked to. Let's copy the spec example. We have found that if we put out documentation with a classname that sounds too nerdy or "official", implementers treat them like magic strings and start supporting those exact classnames. So using my-active-item
helps to not reinforce this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I changed this for the second test in #148 (but see below — maybe we are deleting that test?).
<link rel="dcterms:rights" href="https://www.w3.org/Consortium/Legal/2015/copyright-software-and-document"/> | ||
<link rel="dcterms:rightsHolder" href="https://www.w3.org"/> | ||
<meta property="belongs-to-collection">should</meta> | ||
<meta property="dcterms:isReferencedBy">https://w3c.github.io/epub-specs/epub33/rs/#sec-rsconf-rendering-audio</meta> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<meta property="dcterms:isReferencedBy">https://w3c.github.io/epub-specs/epub33/rs/#sec-rsconf-rendering-audio</meta> | |
<meta property="dcterms:isReferencedBy">https://www.w3.org/TR/epub-rs-33/#sec-rsconf-rendering-audio</meta> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but there are five separate normative statements in this section, and I think (?) this test only tests the four MUSTs (which is by itself fine). Each normative statement should have its own anchor, so please add more anchors in the spec, then include I guess four isReferencedBy elements here.
This is kinda documented at step 12 of https://w3c.github.io/epub-tests/contributing#step-by-step, although we should be more explicit that every normative statement needs a separate anchor (otherwise we can't tell what's tested and what isn't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I was just focusing on the MUSTs because I thought that was what we needed to tackle first. I can write test for that last SHOULD (will prob be a separate EPUB since that test does not have anything in common with the others).
And when I add the test references to the normative statement, as per step 12, I use the test IDs, but where should these be defined for this situation of having multiple tests in one EPUB?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that situation, you would have a single epub but multiple data-tests attributes pointing to the same test, like so:
First normative statement.
Second normative statement.
Third normative statement.
And then mol-rendering-audio.epub would have three dcterms:isReferencedBy:
https://www.w3.org/TR/epub-rs-33/#first-anchor
https://www.w3.org/TR/epub-rs-33/#second-anchor
https://www.w3.org/TR/epub-rs-33/#third-anchor
Make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And you can't refer to a test below the package level of the test EPUB. Is that right?
It doesn't matter at this point for me but conceptually I am curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, missed this. Yes, that's correct.
<meta property="dcterms:isReferencedBy">https://w3c.github.io/epub-specs/epub33/rs/#sec-rsconf-rendering-audio</meta> | ||
<meta property="dcterms:modified">2022-02-10T00:00:00Z</meta> | ||
<meta property="media:duration" refines="#smil-1">00:00:18.479</meta> | ||
<meta property="media:duration">00:00:18.479</meta> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent indentation again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #148.
</head> | ||
<body id="body"> | ||
<h1>EPUB Media Overlays Tests: Rendering Audio</h1> | ||
<p>Each sentence below should play in full when this page is played.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should explicitly list the criteria for the test passing; pass/fail shouldn't require interpretation on the part of the tester (who will hopefully be a RS person not familiar with individual tests).
Put another way, you have this from the perspective of the document (X is not specified, so its value is assumed to be Y) rather than the behavior (X is not specified, so its value is assumed to be Y, and thus Z must occur).
See examples from Ivan's reverted tests in b3ae281.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #148.
<dc:coverage>Media Overlays</dc:coverage> | ||
<dc:creator>Marisa DeMeglio</dc:creator> | ||
<dc:date>2022-02-10</dc:date> | ||
<dc:description>Tests whether the Reading System implements Media Overlays' RS Spec "Rendering Audio" section.</dc:description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with the other package description, please describe the behavior under test rather than just the section of the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #148.
tests/mol-navigation/EPUB/ch2.xhtml
Outdated
</head> | ||
<body id="body"> | ||
<h1 id="mo-1">Chapter 2</h1> | ||
<p id="mo-2">This page should play when "Chapter 2" is selected from the table of contents.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, I'd recommend something explicit like "Test passes if the media overlay audio on this page plays when..." (and does the reader need to enable MO on the first page before doing that? If so, please say so.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instructions start on chapter 1, please see that page first and then let me know if it still doesn't make sense.
I will update the wording to say "The test passes if..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had seen that. The issue is that the chapter 1 and chapter 2 text focus on telling you what to do, not what the pass/fail criteria are. Explicit "test passes if" would help, but for example Ivan's tests say that the RS needs to prompt you to start the MO playing; my belief is it shouldn't be automatic, but I think you can read chapter 1+2 here as the whole thing should be automatic. Since the tester won't have just read the relevant sections of the spec and won't have context, they could misinterpret the criteria; this should be clear to someone who has no context. (I'd copy language from Ivan's tests. We don't want the testers to wonder why one test has different instructions — do those genuinely indicate something different, or just that two different people wrote the tests? Our authorship shouldn't matter.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Marisa did this.)
I think that having multiple tests in a single EPUB, as in |
Well... I have already removed those tests from the respective PR and merge that one. I guess we could find those somewhere in github's memory... (or my mac backups) @marisademeglio what do you prefer the next action would/should be? |
I would do whatever is faster :) This week I don't have much time to spend on this, unfortunately. |
Ok, I have re-instated the three tests for clipend and clipbegin, see #125. @marisademeglio you can then remove the corresponding test from this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Very sorry for the slow reviews here — can you summarize the current state of this PR for me? Is it waiting on my re-review, or do you have changes you need to make first?
(I could probably figure this out myself by careful reading, but I have other PRs to catch up on too.)
Also, attached is a second comment that apparently I wrote weeks ago and neglected to post. I hope it's still relevant!
tests/mol-navigation/EPUB/ch2.xhtml
Outdated
</head> | ||
<body id="body"> | ||
<h1 id="mo-1">Chapter 2</h1> | ||
<p id="mo-2">This page should play when "Chapter 2" is selected from the table of contents.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had seen that. The issue is that the chapter 1 and chapter 2 text focus on telling you what to do, not what the pass/fail criteria are. Explicit "test passes if" would help, but for example Ivan's tests say that the RS needs to prompt you to start the MO playing; my belief is it shouldn't be automatic, but I think you can read chapter 1+2 here as the whole thing should be automatic. Since the tester won't have just read the relevant sections of the spec and won't have context, they could misinterpret the criteria; this should be clear to someone who has no context. (I'd copy language from Ivan's tests. We don't want the testers to wonder why one test has different instructions — do those genuinely indicate something different, or just that two different people wrote the tests? Our authorship shouldn't matter.)
@marisademeglio I wonder where we are with these tests... It would be good to close all outstanding issues and tests before going to CR (which does not mean that we would not need new tests, of course) |
Of the 4 tests in this PR, we are not using the 3 media clipping tests, so that just leaves
The test currently says "While Chapter 1 is playing...", which does not make any assumptions on how playback is initiated or if it's manual or automatic (all this is therefore implementation-dependent - nowhere in the spec(s) do we say how an RS should initiate MO playback though it is generally a "play" button and whether "prompt you to start the MO playing" is the absolute best language to refer to a play button, is a rabbithole I am not going down :) Anyway my opinion is that it's fine as-is. |
@marisademeglio any reason why this PR should not be merged? |
No objection from me, I don't have any plans to make more changes. |
Sorry, this is on me. I've repeatedly intended to re-review but kept not doing so. Lemme go get another coffee and then I shall! I'll commit once I'm happy. (And if I still want some changes, I'll just commit and send a followup PR, because I've kept Marisa waiting waaaaay too long on this one — sorry again!) |
Ah, a bunch of the comments I left in the last review in February have not been addressed, but I will commit this PR anyway and then send a followup PR to make those changes. |
The decision as I understood it was to not use these rendering-audio tests and use Ivan's instead, so I didn't bother to address what you brought up related to rendering-audio. As for your comment about the navigation test, I did reply already. I think then the only thing to do is remove the rendering-audio tests from this PR? Looking back, I see Ivan mentioned that, and I ran out of time to take care of it. |
A-ha! I finally understand. Sorry, I hadn't read the comment threads carefully enough. I thought that your mol-rendering-audio was staying and Ivan's was already gone. Now I see it's the converse. I just cleaned up that test in #148, but I will now send another commit to remove it. |
Three tests for this subsection: https://w3c.github.io/epub-specs/epub33/rs/#sec-rsconf-rendering-audio