Convert unit tests to Jasmine #78

Merged
merged 46 commits into from Feb 20, 2013

Conversation

Projects
None yet
4 participants
Owner

bramstein commented Jan 18, 2013

This pull request converts all unit tests from JSTestDriver to Jasmine. Currently a work in progress.

@webdev webdev and 2 others commented on an outdated diff Jan 18, 2013

spec/javascripts/core/cssclassname-spec.js
@@ -0,0 +1,42 @@
@webdev

webdev Jan 18, 2013

Owner

Very general question, why do the files end with -spec.js and not _spec.js. This is a typical Jasmine convention.

@bramstein

bramstein Jan 18, 2013

Owner

The -spec.js (or rather -test.js) is a Node/Mocha.js convention. I wasn't aware of the Jasmine convention. Shall we change it? I don't have any particular strong opinion on it.

@seanami

seanami Jan 18, 2013

Contributor

Yeah, I think I agree that I'd prefer underscores. We don't have many underscores in this library today, but the few JS files we do have use underscores and we also use that same convention in other projects that use Jasmine.

Owner

bramstein commented Feb 4, 2013

Ideally I'd like to change Jasmine to run from an explicit HTML file before I add support for running it in a headless WebKit. The current setup has too much magic in my opinion, it is much easier to get started for contributors if they can just open an HTML file and run the tests. It probably also integrates better with something like browserstack.com.

Contributor

seanami commented Feb 4, 2013

I very much agree with that approach. We could add a rake task that automatically opens said HTML file in your default browser, just for convenience. But the simplest way to run the tests will be the best for encouraging contributions.

bramstein and others added some commits Feb 5, 2013

@bramstein bramstein Added test for the Kindle Fire 2 to the user agent parser test suite. 3f67b88
Sean McBride Move dev requirements to gemspec, not Gemfile
This is more consistent with the style of developing webfontloader as a
packaged gem that's included by other projects. This also updates all of the
versions of development gems to recent versions.
22566b7
Sean McBride Assume that jasmine is installed by bundler, don't test 1476402
Sean McBride Update the README to reflect the testing updates (and other fixes) 256486c
Contributor

seanami commented Feb 6, 2013

I made a few updates to the gem requirements code, the Rakefile, and the README, just to clean things up a bit. One thing that I wasn't able to figure out: how does Jasmine know where to look for the correct source files and spec files to include? I didn't see a configuration file...

I haven't yet looked through the actual tests, but I'm still planning to. So far, I've just looked through the machinery/setup.

@seanami seanami commented on the diff Feb 11, 2013

desc "Run all tests"
-task :test => ["tmp/jsTestDriver.conf"] do |t|
- config = t.prerequisites.first
- system "java -jar #{JsTestJar} --port #{JsTestPort} --config #{config} --server #{JsTestServer} --browser open --tests all --captureConsole --verbose"
+task :test do |t|
+ exec "phantomjs tools/jasmine-phantomjs/jasmine-phantomjs.js spec/index.html"
@seanami

seanami Feb 11, 2013

Contributor

Should we have two different test tasks? test could be the one that just opens the server with a static HTML page, and then something like test_headless or test_ci could be the one that uses phantom JS. Would that make it easier for people new to the project to run the tests?

@bramstein

bramstein Feb 11, 2013

Owner

It is probably a personal preference, but I usually prefer just opening the HTML file directly and using my own server/tools to start it. I'm also used to make test always being able to run headless.

No strong opinions though, this just made sense to me :)

@seanami

seanami Feb 11, 2013

Contributor

Sounds good to me. Let's just add to the README so it's clear which HTML file to open in order to run the tests, and how.

@seanami seanami and 1 other commented on an outdated diff Feb 11, 2013

spec/ascender/ascenderscript_spec.js
+ key: 'ec2de397-11ae-4c10-937f-bf94283a70c1',
+ families: ['AndyBold', 'Arial:bold,regular']
+ };
+
+ describe('load family and variations', function () {
+ var fakeDomHelper = {
+ insertInto: jasmine.createSpy('insertInto'),
+ createCssLink: jasmine.createSpy('createCssLink'),
+ getProtocol: jasmine.createSpy('getProtocol').andReturn('http:')
+ };
+
+ var fakeOnReady = jasmine.createSpy('onReady');
+
+ var as = new AscenderScript(fakeDomHelper, configuration);
+
+ as.load(fakeOnReady);
@seanami

seanami Feb 11, 2013

Contributor

I think setup like this should be happening in a beforeEach block instead of out in the open at test execution time. Usually, the only thing I place outside of beforeEach or it blocks are var statements so that variables can be shared at the right scope level. Otherwise, the execution time of the setup code isn't explicitly well-defined. Does it all execute when the tests are evaluated, or only once each test is actually run?

@seanami

seanami Feb 11, 2013

Contributor

To clarify, the only thing I place outside an it or beforeEach is a var statement without a value assignment, like var fakeDomHelper;.

@bramstein

bramstein Feb 11, 2013

Owner

Wouldn't that make it harder to operate on an object while testing? e.g.

var a = [1, 2];

// assert that the array exists and is initialized with [1, 2]

a.push(3);

// test that the array now has three elements, etc...

@bramstein bramstein and 1 other commented on an outdated diff Feb 11, 2013

spec/core/domhelper_spec.js
+
+ domHelper.appendClassName(div, 'moo');
+ domHelper.appendClassName(div, 'meu');
+ domHelper.appendClassName(div, 'moo');
+
+ expect(div.className).toEqual('moo meu');
+ });
+
+ it('should add multiple class names', function () {
+ var div = domHelper.createElement('div');
+
+ domHelper.appendClassName(div, 'moo meu moo');
+ expect(div.className).toEqual('moo meu moo');
+ });
+
+ /*
@bramstein

bramstein Feb 11, 2013

Owner

This one is commented out because it is actually broken in real browsers. I don't think it has very high priority because this case shouldn't really occur at the moment (and the worst case is some extraneous white space.)

@webdev

webdev Feb 12, 2013

Owner

You can use xit instead of commenting the block. It'll report a spec as pending.

@bramstein bramstein commented on the diff Feb 11, 2013

spec/monotype/monotypescript_spec.js
+ monotype.supportUserAgent(useragent, support);
+ monotype.load(load);
+
+ global[MonotypeScript.HOOK + configuration.projectId] = function () {
+ return ['aachen bold', 'kid print regular'];
+ };
+
+ script.onload();
+ });
+
+ it('should create a script element', function () {
+ expect(support).toHaveBeenCalled();
+ expect(fakeDomHelper.createElement).toHaveBeenCalledWith('script');
+ expect(script.src).toEqual('http://fast.fonts.com/jsapidev/01e2ff27-25bf-4801-a23e-73d328e6c7cc.js');
+ expect(load).toHaveBeenCalledWith([undefined, undefined], {});
+ });
@bramstein

bramstein Feb 11, 2013

Owner

I removed some of the tests here because I didn't quite get their point. It seemed literally copied from the typekit tests and didn't really test anything that wasn't tested somewhere else already.

@seanami

seanami Feb 12, 2013

Contributor

Sounds reasonable. It was likely the case when this was written.

@bramstein bramstein and 1 other commented on an outdated diff Feb 11, 2013

spec/index.html
@@ -0,0 +1,85 @@
+<!doctype html>
+<html>
+ <head>
+ <title>Webfontloader tests</title>
+ <link rel="stylesheet" href="../tools/jasmine/jasmine.css">
+ </head>
+ <body>
+ <script src="../tools/jasmine/jasmine.js"></script>
+ <script src="../tools/jasmine/jasmine-html.js"></script>
+ <script src="../tools/jasmine-phantomjs/console-reporter.js"></script>
+
+ <script src="../src/core/namespace.js"></script>
@bramstein

bramstein Feb 11, 2013

Owner

All the script includes here are temporary. Once this is merged in I plan to use change to code to use explicit dependencies (e.g. goog.require and goog.require and use a utility (calcdeps.py/calcdeps.js/calcdeps.rb) to generate a dependency file. We can then use the Google dependency loader in development and use the Closure Compiler's dependency resolution to compile out unwanted code.

@seanami

seanami Feb 12, 2013

Contributor

Sounds great!

@seanami

seanami Feb 12, 2013

Contributor

Caveat: We'll want to think about how that dependency system will play with the Typekit code that includes this JS or other code that might have this library's source as a dependency. Will it require the other users of this code to also change dependency systems?

@bramstein

bramstein Feb 13, 2013

Owner

The dependency system will not interfere with that, they are only indications for an external tool to build a dependency tree. You can still include the files the way we've done before. We'll only need to add a small shim to make the goog.provide and goog.require do nothing.

@seanami

seanami Feb 14, 2013

Contributor

Got it, that sounds great then!

Contributor

seanami commented Feb 14, 2013

I've looked through the setup and tested it out locally using both open spec/index and bundle exec rake test, both of which work great for running the tests. Nice!

I also skimmed through all of the tests, and I think with the style changes you made, this is looking good! It's really hard to tell that every test case is definitely covered since there's so much code, but I trust that you did that as you were going through, but nothing jumped out at me.

I did notice that there's currently one failing test:

#appendClassName : should normalize spaces and tabs
Error: Expected 'meu       foo ' to equal 'meu foo'.
DomHelper #appendClassName: 3 of 4 passed.

We should fix that, but otherwise this is looking great to me. It will be so great to have a nicer testing framework with meaningful error output, and an easy way to run them!

seanami closed this Feb 14, 2013

seanami reopened this Feb 14, 2013

Contributor

seanami commented Feb 14, 2013

Whoops, sorry about that. Clicked the wrong button. This is still open.

@raziel raziel commented on the diff Feb 15, 2013

spec/core/cssfontfamilyname_spec.js
+ });
+
+ it('should quote names with spaces and single quotes', function () {
+ expect(sanitizer.quote("'My Family'")).toEqual("'My Family'");
+ });
+
+ it('should quote multiple single quoted names separated with a comma', function () {
+ expect(sanitizer.quote("'family 1','family 2'")).toEqual("'family 1','family 2'");
+ });
+
+ it('should quote multiple single quoted names separated with a comma and space', function () {
+ expect(sanitizer.quote("'family 1', 'family 2'")).toEqual("'family 1','family 2'");
+ });
+
+ it('should not quote when there is no space', function () {
+ expect(sanitizer.quote('MyFamily')).toEqual('MyFamily');
@raziel

raziel Feb 15, 2013

Collaborator

small thing, moving forward: negated specs should probably be written using the "not" operator to match the text above.

@bramstein bramstein added a commit that referenced this pull request Feb 20, 2013

@bramstein bramstein Merge pull request #78 from typekit/bs-jasmine
Convert unit tests to Jasmine
90b3933

@bramstein bramstein merged commit 90b3933 into master Feb 20, 2013

bramstein deleted the bs-jasmine branch Feb 20, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment