Skip to content

singleimagedisplay unit test #35

Merged
merged 8 commits into from Dec 20, 2011

2 participants

@davidmaxwaterman
web-ui-fw member

ssia

This branch is quite far behind master, but most changes are to singleimagedisplay. I seem some changes to dist/ files have slipped in, so I guess those might need some reverting.

Don't blindly merge.

@davidmaxwaterman
web-ui-fw member

There is quite a lot of common/similar code in the unit test; I was wondering if I should refactor it.

@townxelliot

Nice, Max. Very comprehensive.

I pushed a commit to this branch to add your suite to the overall test suite.

Could I suggest you rebase on master, as dist/ has been removed from there?

My one comment is that we should round the aspect ratios before doing comparisons: I was getting precision errors on some of the equal() assertions, causing the tests to fail (in Chrome). The amount the comparison was out was typically around 0.016 or so, e.g.

```aspect ratio correct
Expected:

5
Result:

5.015706806282722
Diff:

5 5.015706806282722
Source:

at HTMLImageElement. (file:///home/ell/dev/html5/web-ui-fw/tests/coverage/instrumented/singleimagedisplay/singleimagedisplay.js:580:21)


I suggest using Math.floor on the numbers (presuming they should come out as whole numbers).
@townxelliot townxelliot and 1 other commented on an outdated diff Dec 20, 2011
...s/unit-tests/singleimagedisplay/singleimagedisplay.js
@@ -0,0 +1,681 @@
+/*
+ * singleimagedisplay unit tests
+ */
+
+(function ($) {
+
+ var func = [
@townxelliot
townxelliot added a note Dec 20, 2011

Hmm, not really keen on unnamed functions. There is also still a lot of repetition here (1 and 2 are very similar). I'm not even sure this has reduced the line count compared to the version where you just had the same code cut and pasted into each test.

I'm actually inclined to suggest returning to the version where you hadn't refactored the code into functions, as it was actually easier to follow.

@davidmaxwaterman
web-ui-fw member

The line count was reduced from 824 to 681 - some of the functions are reused several times (func[0] is used 7 times and the overhead is minimal. Several are only used once, but I figured it was better to have a consistent pattern.
Hrmph. Looking into this, I notice [3] isn't used at all...that'll save some more.

I'm not keen on anonymous functions either, but was lacking inspiration. In any case, I agree that 'func' isn't a great name - I did have it in mind to change that, but forgot. "resizeTest[0]", for example, might be better.

I agree that having the tests inline was easier to follow. Would you prefer me to put it back?

@davidmaxwaterman
web-ui-fw member

no problem...I wasn't certain quite where it was going anyway. I'll turn the clock back.

@davidmaxwaterman
web-ui-fw member

ok, I deleted that last commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@townxelliot townxelliot merged commit 3e06556 into master Dec 20, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.