Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Two tests failing #939

Closed
danse opened this Issue Dec 4, 2012 · 19 comments

Comments

4 participants

danse commented Dec 4, 2012

Two tests are failing for me:

'selects by class name' into selectAll-test.js:22
'selects by class name' into select-test.js:22

They succeed after the following changes:

diff --git a/test/core/select-test.js b/test/core/select-test.js
index 924ceac..56b6cca 100644
--- a/test/core/select-test.js
+++ b/test/core/select-test.js
@@ -19,7 +19,7 @@ suite.addBatch({
     },
     "selects by class name": function() {
       var div = d3.select(".foo");
-      assert.equal(div[0][0].className, "foo");
+      assert.equal(div[0][0].className, "foo bar");
     },
     "selects by id": function() {
       var div = d3.select("div#bar");
diff --git a/test/core/selectAll-test.js b/test/core/selectAll-test.js
index 9ac4646..633596a 100644
--- a/test/core/selectAll-test.js
+++ b/test/core/selectAll-test.js
@@ -19,7 +19,7 @@ suite.addBatch({
     },
     "selects by class name": function() {
       var div = d3.selectAll(".foo");
-      assert.equal(div[0][0].className, "foo");
+      assert.equal(div[0][0].className, "foo bar");
     },
     "selects by id": function() {
       var div = d3.select("div#bar");

But I think that the meaning of the test was exactly to be sure that the class name is just "foo".

My versions:
node: 0.8.15
d3: current ( 44e4620 )

Output of npm freeze:
"canvas@0.13.0" : ""
"jsdom" : "0.2.14"
"sizzle" : "1.1.0"
"uglify-js" : "1.3.3"
"vows" : "0.6.4"

By the way, what do you think about adding a travis file to the repository? Should I propose one?

Owner

mbostock commented Jan 23, 2013

Unfortunately, I'm not able to reproduce, but I wouldn't be too surprised if there was a subtle ordering dependency in the tests. I've tried to make them order-independent but that can be a bit difficult if they are operating on a shared DOM.

danse commented Jan 24, 2013

It does not looks to me like an order dependency issue.

The setup function is correctly empting the body before each test, and in both the cases, https://github.com/mbostock/d3/blob/master/test/core/select-test.js and https://github.com/mbostock/d3/blob/master/test/core/selectAll-test.js the element.className method is involved.

This is the way the tests fail for me,

    selectAll 
      ✗ selects by class name 
        »        
        actual expected 

        foo bar 
         // selectAll-test.js:22 

    select 
      ✗ selects by class name 
        »        
        actual expected 

        foo bar 
         // select-test.js:22 

"foo bar" is returned, "foo" was expected. So it looks like the className methods also returns the element "id" together with the name. This is not surprising me given the reliability of DOM methods, but I have not idea about why this is failing just for me.

Contributor

ZJONSSON commented Jan 24, 2013

Would it not make sense to allow d3.select to judge the dom-context from the selected (non-string) object if applicable, otherwise global document? Obviously this might mean a lot of replacement in the core where document is referred to as a global variable, not a local variable of the given selection.

Owner

mbostock commented Jan 24, 2013

I've just upgraded all of D3’s test dependencies (upgrading JSDOM & Vows, removing Sizzle, etc.). Would you mind upgrading to D3 3.0.5, or pulling if you have cloned the repository, and then npm update (or npm install, I forget which)? And then let me know if this fixes the problem for you.

danse commented Jan 25, 2013

Nope, same problem.

node v0.8.15
d3 3.0.5
npm freeze :
"jsdom@0.3.4" : ""
"uglify-js" : "2.2.3"
"vows" : "0.7.0"

Since it is a problem with setAttribute and className at the end, I could be able to build a reduced test case as an issue for JsDOM, when I will have time.

Owner

mbostock commented Jan 25, 2013

Do you have either of these environment variables set, which could interference?

NODE_PATH ?= ./node_modules
LOCALE ?= en_US
Owner

mbostock commented Jan 25, 2013

I was running node 0.8.6; I upgraded to node the latest 0.8.18 and make test and npm test still work perfectly for me.

danse commented Jan 25, 2013

No, I don't have those variables set, but I see that they can affect the makefile. I am not running any make command before the tests, just npm install and npm test as in the docs .

danse commented Jan 25, 2013

Well looks like I was missing the necessary step. I'm on an old machine now, I will retry in some days, but I added the make step into docs at https://github.com/mbostock/d3/wiki

danse commented Jan 28, 2013

Fine, after making, those two tests are not failing anymore, but other 30 tests are failing. I'm not pasting here the failures, it will mess up this discussion, and it is not meaningful since the colours from vows get lost.

I don't know whether it is normal having those tests failing, or if they are all running perfectly on your environment. What do you think about adding a travis file? That would be useful for anyone as a reference of the current test status and the needed configuration.

I'm also seeing "selects by class name" failing. I'm using node 0.8.18. I checked out a new fork and ran npm install.

I'm having a problem with the NodeJS globals not loading correctly. The delete global[g]; line in index.js is deleting the jsdom globals after module loads them (I'm putting together a pull request). I was wondering if this is somehow related since the tests also use jsdom, but so far it doesn't look like it.

Owner

mbostock commented Jan 31, 2013

The delete global[g] doesn’t get run in the test environment. The test environment loads d3 via env.js, not index.js.

danse commented Feb 1, 2013

I think this issue has no meaning anymore. The tests in the title are no more failing for me, so the issue is somehow resolved, but I think that the need for a public test service has emerged, as a reference for anyone, in order to avoid confusing, wrong assumptions. I'm referring to Travis. I'm not opening a new issue, since #503 already existed, so maybe it could be opened again, and if the case host a debate about this topic.

Owner

mbostock commented Mar 7, 2013

I believe this is fixed in the 3.1.0 branch. I cannot reproduce, but please let me know if you're still seeing errors by reopening this bug after trying the 3.1.0 branch.

@mbostock mbostock closed this Mar 7, 2013

danse commented Mar 7, 2013

I tried. I see the state as it was about one month ago (see above), 31 tests failing on branch 3.1.0, including select and selectAll. I lost the thread now: are we talking about the two tests? What about the others? I'm perfectly ready to blame myself for some error in my environment setup, but i bid you to accept the proposal of a travis file from someone, in order to have a reference test environment for all. Do you see any drawbacks with this?

Owner

mbostock commented Mar 7, 2013

Could you try blowing away the local node_modules in your d3 repository, followed by a clean npm install? (I know I asked you to do this previously and it had no effect, but there were recent changes to the package.json and I want to at least isolate the effect of these changes.)

I've added a .travis.yml file… I'd rather not give Travis write-access to the d3 repository, though, so I'm not sure that this will have any effect.

danse commented Mar 8, 2013

I apologize, some global package was actually affecting the tests! After removing the global node modules folder from path, now on the 3.1.0 and on master branch just two tests fail, select and selectAll! But far better anyway. So i was somehow guilty, i come from python and i did not found with node a way of handling modules which is convenient as python virtual envs. But maybe i'm just not updated about the last npm features.

About the travis service, i don't think it needs write access to the repository, just read access. It is just a git hook after commits. Then you can add a badge in the readme file, which will show to anyone if the current master is passing or failing the tests. But most of all, this badge will show users that a travis job is set up, and so they can see the setting and reproduce the environment without having to bother you! If one day you will want to do a release with some failing tests, anybody will know which tests have known failures.

@mbostock mbostock reopened this Mar 14, 2013

Owner

mbostock commented Mar 14, 2013

In the process of refactoring the tests, I am now able to reproduce these errors. The issue is that some of the tests are not isolated from each other, and so their behavior is order-dependent. I will attempt to isolate these tests now. (You can verify this using vows -i when running tests.)

mbostock added a commit that referenced this issue Mar 14, 2013

Owner

mbostock commented Mar 14, 2013

Staged in #1120.

@mbostock mbostock closed this Mar 14, 2013

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