More tests for Microformat API

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: cmtalbert, Assigned: cmtalbert)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 314000 [details] [diff] [review]
The tests

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?
(Assignee)

Comment 2

11 years ago
(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
(Assignee)

Comment 4

11 years ago
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.
(Assignee)

Comment 5

11 years ago
Created attachment 316464 [details] [diff] [review]
Tests in a patch

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

Comment 7

11 years ago
Created attachment 318967 [details] [diff] [review]
new patch with frame recursion changes and the corrections

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)
(Assignee)

Comment 8

11 years ago
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+
(Assignee)

Comment 10

11 years ago
Created attachment 322170 [details] [diff] [review]
Fixes the path in the makefile for proper test publishing

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+
(Assignee)

Comment 11

11 years ago
Fix checked in on Moz Central.  Should I also check it in on CVS?
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

11 years ago
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?
(Assignee)

Comment 14

11 years ago
Thanks Gavin.  Checked in on CVS trunk too.
You need to log in before you can comment on or make changes to this bug.