Closed Bug 427457 Opened 16 years ago Closed 16 years ago

More tests for Microformat API

Categories

(Toolkit Graveyard :: Microformats, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: cmtalbert, Assigned: cmtalbert)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch The tests (obsolete) — Splinter Review
These are some more tests I've had in the works for a while surrounding the microformat API's.  I think most of them are pretty explanatory.  There is a bit of a dependency on bug 427456, due to the recurseFrame weirdness, so I'll mark that as blocking this bug since however that is resolved will un"todo" some of these tests.  

Also, I have two questions about adding a third party microformat:
* I'm having trouble accessing an embedded "standard" microformat in my microformat.  It keeps coming back as undefined (see test_Microformats_add.html: line 208)
* It seems from line http://mxr.mozilla.org/mozilla/source/toolkit/components/microformats/src/Microformats.js#270  That extension authors are going to have to know what the internal version is of the Microformats extension.  It seems that this line really wants to compare the new version for the added microformat with the existing version of that microformat if it exists, something more like:
if (microformatDefinition.mfVersion == Microformats[name].version)

However, this may not be the case.  If the extension authors need to know the internal version of the microformat API then this must be documented somewhere, and I'll open a follow on bug for that.

Attaching the testcases.

Mkaply, if you have time, would you mind looking over them and giving me feedback on these above questions before I submit them for formal review?
Assignee: nobody → mozilla
Component: General → Microformats
Product: Firefox → Toolkit
QA Contact: general → microformats
Thanks for this. This is pretty cool.

I think the problem with your undefined microformat is your geo isn't correct:

+      <abbr class="geo">
+        <span class="latitude" title="659"/>
+        <span class="longitude" title="-362.5"/>
+      </abbr>

should be

+      <span class="geo">
+        <span class="latitude">659</span>
+        <span class="longitude">-362.5</span>>
+      </span>

or

+      <abbr class="geo" title="659,-362.5">Geo</abbr>

As far as the version thing goes, you're right that it isn't defined well.

I'm wondering if I should just remove the check completely. I don't foresee changing the layout of these things any time soon.

Based on your interaction with my API, do you feel it is pretty baked?
(In reply to comment #1)
> Thanks for this. This is pretty cool.
> 
My pleasure, I'm sorry they came in so late.  Your unit tests looked so good, I had to keep pushing these back and focused my energy on items with less test coverage :-(

> I think the problem with your undefined microformat is your geo isn't correct:
> 
> +      <abbr class="geo">
> +        <span class="latitude" title="659"/>
> +        <span class="longitude" title="-362.5"/>
> +      </abbr>
> 
> should be
> 
> +      <span class="geo">
> +        <span class="latitude">659</span>
> +        <span class="longitude">-362.5</span>>
> +      </span>
> 
> or
> 
> +      <abbr class="geo" title="659,-362.5">Geo</abbr>
> 
Ah, I'll try that in my next version of the patch.  Thanks.

> As far as the version thing goes, you're right that it isn't defined well.
> 
> I'm wondering if I should just remove the check completely. I don't foresee
> changing the layout of these things any time soon.
I think it'd be smart to remove it, but if you decide to keep it, could we make it a constant on the interface?  MICROFORMAT_VERSION or some such...that might make it easier to use for extension authors.
> 
> Based on your interaction with my API, do you feel it is pretty baked?
> 
It feels really solid.  The only things that tripped me up were the version number and the frame recursion bit that we have in that other bug.  Everything else was very straightforward.  Nice work!
Clint: I'm removing the internal version in bug 429151
Awesome!(In reply to comment #3)
> Clint: I'm removing the internal version in bug 429151
> 
Awesome!  I'll wait on that patch to land and will then submit a new patch of tests for formal review.
Attached patch Tests in a patch (obsolete) — Splinter Review
These are the tests.  The *_add test still fails to find the embedded geo microformat in the hTest MF that I added.  Everything else works.
Assignee: mozilla → ctalbert
Attachment #314000 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #316464 - Flags: review?(mozilla)
One of the embedded geos is my bug. See bug 430466.

The other two geos are not formed correctly:

+      <span class="geo">
+        <abbr class="latitude">0.0</abbr>
+        <abbr class="longitude">0.0"</abbr>
+      </span>

should be

+      <span class="geo">
+        <span class="latitude">0.0</span>
+        <span class="longitude">0.0"</span>
+      </span>


and

+      <span class="geo">
+        <abbr class="latitude">659</abbr>
+        <abbr class="longitude">-362.5</abbr>
+      </span>

should be

+      <span class="geo">
+        <span class="latitude">659</span>
+        <span class="longitude">-362.5</span>
+      </span>

Also in that case, note that 659;-362.5 is an invalid lat/long - they must be between -360 and +360.

Otherwise this is great.
Here is the final version of the patch with the "recurseExternalFrame" changes and the changes for the geos.

w.r.t. 659;-362.5 geo, I intentionally created that geo with those wacky coordinates because I wanted to test an invalid Microformat.  However, I didn't intend to test an invalid Microformat by invalidating the structure of the Microformat.  So I corrected the structure of that geo and left the wacked out coordinates in place.

I corrected the other geo's properly.
Attachment #316464 - Attachment is obsolete: true
Attachment #318967 - Flags: review?(mozilla)
Attachment #316464 - Flags: review?(mozilla)
Oh and I tested it with the new changes from bug 427456, and all tests pass. :-)
Comment on attachment 318967 [details] [diff] [review]
new patch with frame recursion changes and the corrections

r=mkaply
Thanks for doing this.
Only suggestion is that I put the tests in the wrong output dir to begin with.

$(INSTALL) $^ $(DEPTH)/_tests/testing/mochitest/tests/browser/microformats/test

should probably be

$(INSTALL) $^ $(DEPTH)/_tests/testing/mochitest/tests/toolkit/components/microformats/test
Attachment #318967 - Flags: review?(mozilla) → review+
Same patch, now with a better publishing path for the test files (they end up in mochitests/tests/toolkit/components/microformats/tests
Attachment #318967 - Attachment is obsolete: true
Attachment #322170 - Flags: review?(mozilla)
Attachment #322170 - Flags: review?(mozilla) → review+
Fix checked in on Moz Central.  Should I also check it in on CVS?
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
I'm marking the wanted flag to know if we want to also add these tests to 1.9.0.x...
Flags: wanted1.9.0.x?
Yeah, please go ahead and land on CVS trunk (assuming they all pass there).

You don't need approval, per http://wiki.mozilla.org/TreeStatus .
Flags: wanted1.9.0.x?
Thanks Gavin.  Checked in on CVS trunk too.
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: