fluid-work IRC Logs-2012-10-18

[00:00:02 CDT(-0500)] <thealphanerd> geometry of music

[00:00:03 CDT(-0500)] <thealphanerd> dope

[09:07:51 CDT(-0500)] <jhung> justin_o cindyli - I had a capturer.html file in the capture/html directory. Did I forget to add it or was it deleted?

[09:08:22 CDT(-0500)] <Justin_o> jhung: i moved some stuff around.. i made that into a template

[10:04:22 CDT(-0500)] <anastasiac> michelled, I've issued a pull request for FLUID-4775, cindy's branch to get the video player working in IE8. Master does not work at all; this branch does. It still has some problems, but we could file individual JIRAs for those problems

[10:04:47 CDT(-0500)] <michelled> thanks anastasiac

[10:06:54 CDT(-0500)] <alexn> michelled: when you get a chance, could you review another branch for OER Commons https://github.com/anvk/OER-Commons/compare/ISKME:staging...OER-797

[10:07:18 CDT(-0500)] <alexn> it is for the issue https://www.assembla.com/spaces/iskme/tickets/797

[10:07:40 CDT(-0500)] <michelled> sure alexn

[10:07:58 CDT(-0500)] <alexn> michelled: thx!

[10:08:01 CDT(-0500)] <michelled> can you add it to the planning page if you haven't already?

[10:09:21 CDT(-0500)] <alexn> michelled: sure. I will update planning page accordingly

[10:09:32 CDT(-0500)] <michelled> thx

[14:19:56 CDT(-0500)] <Bosmon> michelled - looking at your pull request 74

[14:20:19 CDT(-0500)] <michelled> thx Bosmon

[14:20:23 CDT(-0500)] <Bosmon> You're right that, in the case of an asynchronous test, the teardown fn is the appropriate place to do the environment cleanup

[14:20:49 CDT(-0500)] <Bosmon> But as a result of that, you should change the implementation to be consistent, and also do the environment setup in the setup function

[14:21:37 CDT(-0500)] <michelled> Bosmon: but each test requires a different setup and I can't provide a different setup for each test

[14:21:49 CDT(-0500)] <Bosmon> Can you explain what you mean?

[14:23:39 CDT(-0500)] <michelled> https://github.com/michelled/videoPlayer/blob/ea68c27971d2a2172e122ada5e9c3a0c8980a0e3/tests/js/VideoPlayerHTML5CaptionatorTests.js#L142

[14:23:52 CDT(-0500)] <michelled> for the first test, we want supportsHTML5 to be null

[14:24:16 CDT(-0500)] <michelled> whereas for the second test we want it to be true

[14:24:25 CDT(-0500)] <michelled> perhaps I didn't understand what you were asking me to do

[14:25:41 CDT(-0500)] <michelled> the setup function gets passed to the test case when we create it and it is run for each test in the test case

[14:26:12 CDT(-0500)] <michelled> the way we've been writing our test cases, we have several tests in them that actually require different setup

[14:26:21 CDT(-0500)] <Bosmon> Well... I guess my point is that it is rather weird that you are setting up the environment in one way, and clearing it in another

[14:26:33 CDT(-0500)] <Bosmon> It's the kind of thing that's likely to lead to confusion in the long run

[14:28:30 CDT(-0500)] <michelled> I guess we can change the way we've been using test case. it would probably be more in line with how it was designed to be used

[14:29:05 CDT(-0500)] <Bosmon> Yes, the qunit guys do seem to be rather opinionated on that front

[14:29:38 CDT(-0500)] <Bosmon> But I guess the issue is that every particular "test instance" needs to use the same setup and teardown functions within a "test case"

[14:30:03 CDT(-0500)] <michelled> yes

[14:30:14 CDT(-0500)] <Bosmon> Or "test instance".... whatever they call the damn things

[14:30:42 CDT(-0500)] <Bosmon> Given we don't REALLY have the ability to customise the setup per "test fixture" we shouldn't suggest in the way that we issue our configuration that we can

[14:32:04 CDT(-0500)] <michelled> right, so testCaseWithEnv would take and env instead of having that nested inside the testCaseInfo structure

[14:32:29 CDT(-0500)] <michelled> then I don't need to collect it up

[14:32:38 CDT(-0500)] <Bosmon> I think that seems more natural?

[14:32:39 CDT(-0500)] <Bosmon> What do you think

[14:33:07 CDT(-0500)] <michelled> I think it does, although I'm beginning to get a lot of parameters to the function

[14:33:14 CDT(-0500)] <Bosmon> Which one?

[14:33:32 CDT(-0500)] <michelled> testCaseWithEnv: name, env, testCaseInfo, setupFn, teardownFn

[14:34:30 CDT(-0500)] <Bosmon> Yes

[14:34:42 CDT(-0500)] <Bosmon> You might want to think about reforming it to take an options structure

[14:35:22 CDT(-0500)] <michelled> well, it seems to me that name, env, and testCaseInfo are required - do you agree?

[14:35:31 CDT(-0500)] <michelled> setup and teardown could go in options

[14:35:59 CDT(-0500)] <Bosmon> Yes, they are required - but it doesn't mean they can't go in options

[14:36:41 CDT(-0500)] <michelled> I'd rather be explicit with requirements

[14:36:52 CDT(-0500)] <michelled> it'll be such a pain to track down tests with no name

[14:37:26 CDT(-0500)] <Bosmon> Will it be any easier or harder, if the name is in an argument or an option?

[14:37:52 CDT(-0500)] <michelled> I think it'll be less likely for something to call the function without the name if the name is an argument

[14:38:09 CDT(-0500)] <michelled> I'm arguing for clarity for the test writer

[14:38:30 CDT(-0500)] <Bosmon> Ok - I think either all the arguments should be in options, or none of them.... so if you don't like the options idea, you should carry on with the current signature

[14:38:41 CDT(-0500)] <Bosmon> Although it's a little long, it's actually no different to the existing qunit signature

[14:39:13 CDT(-0500)] <michelled> besides adding the env I guess

[14:40:10 CDT(-0500)] <Bosmon> Interestingly in the qunit implementation there already is a thing called "testEnvironment"...

[14:41:36 CDT(-0500)] <Bosmon> Although it seems to really just include "everything"

[14:45:11 CDT(-0500)] <Bosmon> Well, it doesn't seem to be used for much, other than the setup/teardown functions

[14:45:22 CDT(-0500)] <Bosmon> Although it does become the "this" used when the callback is invoked

[14:47:09 CDT(-0500)] <Bosmon> And who knew that the 1st argument of the callback is, for some reason, QUnit.assert! : P

[14:49:04 CDT(-0500)] <yura> Bosmon: hi

[14:49:29 CDT(-0500)] <yura> Bosmon: could you review my pull request https://github.com/GPII/universal/pull/56 for avtar's deployment, it is really small

[14:53:28 CDT(-0500)] <Bosmon> michelled - you may be interested in yura's pull request too

[14:53:48 CDT(-0500)] <yura> oh noes

[14:53:49 CDT(-0500)] <Bosmon> Which "points to the future" in that in the future, noone will be making function calls : P

[14:53:50 CDT(-0500)] <yura> what did i do

[14:54:11 CDT(-0500)] <michelled>

[14:54:21 CDT(-0500)] <yura> I came from the future to drop this pull request on you

[14:54:29 CDT(-0500)] <Bosmon> It's greatly appreciated

[14:54:56 CDT(-0500)] <Bosmon> It seems in the future, we are very fond of "cuteness"

[14:55:04 CDT(-0500)] <yura> Also in case anyone needs some lottery numbers

[14:55:24 CDT(-0500)] <Bosmon> And write constructions like this, where a simple string concatenation would suffice

[14:55:25 CDT(-0500)] <Bosmon> [resp.error, resp.reason].join(": ")

[14:56:05 CDT(-0500)] <yura> Bosmon: that's not what they say in the future

[14:56:10 CDT(-0500)] <Bosmon> hahaha

[14:57:31 CDT(-0500)] <Bosmon> I guess in the future, they are also uninterested in the garbage costs of an extra array object

[14:57:49 CDT(-0500)] <Bosmon> I bring you this comment from colinclark, who is in some parallel universe which may or may not be in the future....

[14:57:59 CDT(-0500)] <Bosmon> " screw the cost, it's clever! "

[14:58:18 CDT(-0500)] <yura> great

[14:58:32 CDT(-0500)] <yura> it's an error URI

[14:59:16 CDT(-0500)] <yura> so having triple string concatenation is ok then ?

[15:00:25 CDT(-0500)] <Bosmon> Well certainly the cost argument is an extremely weak one

[15:00:43 CDT(-0500)] <Bosmon> But I think the "cost" and "idiomaticity" arguments point the same way, however weak they are

[15:00:55 CDT(-0500)] <Bosmon> The real effect of this construction is to concatenate EXACTLY three strings

[15:01:04 CDT(-0500)] <Bosmon> So the appearance of generality is really false

[15:01:34 CDT(-0500)] <Bosmon> Also worth bearing in mind that x = a + b + c constructions may very well have special treatment by the JS VM, whereas "join" statements most likely do not

[15:02:06 CDT(-0500)] <yura> agreed

[15:02:19 CDT(-0500)] <Bosmon> In seeing concrete concatenations, the VM can "look ahead" to realise that certain elements can definitely be garbage collected early

[15:02:31 CDT(-0500)] <Bosmon> Or that, for example, a string can be expanded using "realloc"

[15:03:08 CDT(-0500)] <Bosmon> Although the latter would be an extremely aggressive optimisation, given the damn things are meant to be immutable : P

[15:03:11 CDT(-0500)] <Bosmon> but it's not impossible....

[15:03:42 CDT(-0500)] <yura> ok give me one moment

[15:04:16 CDT(-0500)] <Bosmon> I guess it's an unreasonably long discussion to have on such a small point, but I guess in the end the way we reason about these things in general is important

[15:04:44 CDT(-0500)] <Bosmon> Given every piece of code should reflect the best balance between idiom and efficiency, however small that balance is........

[15:04:51 CDT(-0500)] <yura> given couchdb api is not our decision it is fine

[15:05:03 CDT(-0500)] <Bosmon> In this case as I see it, both idiom and efficiency point in the same direction, however weakly

[15:05:52 CDT(-0500)] <Bosmon> Another tiny interesting point is that you and michelled have made different decisions regarding the encoding of the test type

[15:06:04 CDT(-0500)] <Bosmon> You have written testType: "asyncTest",

[15:06:09 CDT(-0500)] <Bosmon> Whereas she has a boolean flag

[15:06:17 CDT(-0500)] <Bosmon> To be honest, I slightly prefer your method....

[15:06:22 CDT(-0500)] <yura> WIN

[15:06:42 CDT(-0500)] <Bosmon> Although it's hard to say why : P

[15:06:50 CDT(-0500)] <yura> well the reason is that boolean implies duality

[15:06:50 CDT(-0500)] <Bosmon> It's true that there ARE only two types of test....

[15:06:58 CDT(-0500)] <Bosmon> Yes, it's an important moral point

[15:07:09 CDT(-0500)] <Bosmon> But actually in qunit, there ARE only two types of test

[15:07:26 CDT(-0500)] <Bosmon> They actually use the boolean flag encoding in their own implementation

[15:07:33 CDT(-0500)] <Bosmon> But we might very well argue that "this is none of our business"

[15:09:50 CDT(-0500)] <Bosmon> This "directModel is null" business is still troubling.... if there is any case where a null model is at all acceptable, we should really review whether the DataSource API is appropriate.... but given this is just a test case it seems harmless enough

[15:09:58 CDT(-0500)] <Bosmon> But I did see it in the actual implementation somewhere too....

[15:10:27 CDT(-0500)] <yura> well you did

[15:10:40 CDT(-0500)] <yura> but i was doing a lot of arguments inference

[15:33:57 CDT(-0500)] <Bosmon> Wow

[15:34:02 CDT(-0500)] <Bosmon> The actual git part of github is down....

[15:35:14 CDT(-0500)] <yura> yes

[15:43:51 CDT(-0500)] <yura> Bosmon: https://github.com/GPII/universal/pull/56 is updated

[15:44:22 CDT(-0500)] <Bosmon> Yes

[15:44:29 CDT(-0500)] <Bosmon> Now I just need to unjam my hung git process : P

[15:53:53 CDT(-0500)] <michelled> Bosmon: I updated my pull request when you have moment. I didn't change my async flag - I actually think it's clearer than having a 'testType' field in the implementation I have

[15:54:43 CDT(-0500)] <michelled> it doesn't limit us to having other types of tests in the future - it just has a special place to denote whether a test synchronous or asynchronous.

[15:54:56 CDT(-0500)] <Bosmon> michelled - ok

[15:56:23 CDT(-0500)] <michelled> Bosmon: my setup and teardown are looking awful similar - do you think it's worth factoring out the repetition?

[15:56:35 CDT(-0500)] <michelled> or do you think it will just complicate the code for not much gain

[15:56:48 CDT(-0500)] <Bosmon> yura - I merged yours in

[15:57:47 CDT(-0500)] <yura> Bosmon: thanks, avtar^

[16:00:46 CDT(-0500)] <avtar> no, thank you

[16:04:49 CDT(-0500)] <Bosmon> Hi michelled - I don't see an update yet?

[16:05:27 CDT(-0500)] <michelled> really? I pushed it a while ago

[16:05:46 CDT(-0500)] <michelled> https://github.com/michelled/videoPlayer/blob/18a8aa65e71f5b7f3ae339d0601e446af0ffbf1d/tests/js/TestUtils.js

[16:09:01 CDT(-0500)] <Bosmon> Oh, sorry

[16:09:06 CDT(-0500)] <Bosmon> Somehow I was looking at the wrong pull request

[16:09:56 CDT(-0500)] <Bosmon> michelled - no, I don't think it's worth factoring out the repetition

[16:12:39 CDT(-0500)] <michelled> ok, thx Bosmon

[16:12:42 CDT(-0500)] <michelled> so, can I push it?

[16:14:13 CDT(-0500)] <Bosmon> michelled - I'm wondering why the VideoPlayerTests.js doesn't use the new infrastructure you created?

[16:15:26 CDT(-0500)] <michelled> Bosmon: mostly because this pull request was about captions. I was intending on cleaning up the video player tests in another pull request

[16:15:54 CDT(-0500)] <Bosmon> Yes, but you did make a lot of changes in that file - it seems odd that you only rewrote the tests half-way

[16:16:36 CDT(-0500)] <michelled> yes, I considered reverting the changes that alexn has made to that file, but they seemed like an improvement over what was there, so I left that in

[16:16:49 CDT(-0500)] <michelled> do you think I should either finish it or revert it?

[16:16:58 CDT(-0500)] <Bosmon> I think you may as well finish it

[16:17:28 CDT(-0500)] <michelled> ok, I'll do controllers tests too then - although I think I'd better make another JIRA

[16:17:34 CDT(-0500)] <michelled> it's getting a bit ridiculous

[16:18:27 CDT(-0500)] <Bosmon> I think it's reasonable if you make a new piece of infrastructure, to roll the updates in favour of it into the same branch....

[16:19:28 CDT(-0500)] <michelled> I think we should have made a new JIRA for the new piece of infrastructure - but I think it crept up on us. we didn't see it until it already existed

[16:19:43 CDT(-0500)] <michelled> so you think I should continue to use 4787?

[16:20:27 CDT(-0500)] <Bosmon> Certainly it could have a new JIRA, yes