Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Browser support with level.js #20

Closed
wants to merge 0 commits into from
Closed

Conversation

refset
Copy link
Collaborator

@refset refset commented Jun 26, 2013

Initial PR to address #10

@refset
Copy link
Collaborator Author

refset commented Jun 26, 2013

Ah, sorry about that "mend" - can I fix that or is it too late??

@refset
Copy link
Collaborator Author

refset commented Jun 28, 2013

Not sure how to test testling without pestering you with commit notifications... so I'm closing until I'm done :)

@refset refset closed this Jun 28, 2013
@refset
Copy link
Collaborator Author

refset commented Jun 28, 2013

  1. and 4) should be done okay now.

BUT, I'm a little confused about this testing business......

When you said browserling you just meant ci.testling.com, right? Testling will hopefully run the existing mocha tests okay, though I'm not sure why it's not displaying results yet. I based the package.json changes off of https://github.com/substack/mocha-testling-ci-example

As to your point 1) about having browser tests that run locally. Max's level.js does this quite nicely but I don't see how to do this with your mocha tests at all :/ Any pointers?

And is that all that you wanted doing?

Cheers!

@mcollina
Copy link
Collaborator

Yep, right. I meant testling.
The test as they are cannot be run inside a browser, as they are relying on leveldown, the C++ binding of LevelDB.
https://github.com/dominictarr/level-test provides the harness you want. It uses LevelDOWN on node and level.js in the Browser.

To run them locally, you can try running testling directly: https://github.com/substack/testling

Let's pull @substack in, maybe he can help :).

@refset
Copy link
Collaborator Author

refset commented Jun 28, 2013

Running testling locally has been a really fruitful suggestion - I didn't realise it was an option when last commented! I've had to add level-js support to level-test and make a minor modification to level-js, hopefully both will get pulled upstream soon.

43 / 76 tests are passing locally.

31 of the failures are due to timeout errors, and look like:

not ok 1 basic join algorithm should do a join with one results __testling_prelude.js:795
  Error: timeout of 2000ms exceeded
      at http://localhost:60281/node_modules/mocha/mocha.js:4019:14 

...from navigator_spec.js and abstract_join_algorithm.js

I think the real culprits are the use of .join and .nav, but I haven't taken a peak inside yet.

The other two failing tests are to do with the level-js .del not working :/

Anyway, I SWEAR TO GOD that's it for this week's attempt ;)

@mcollina
Copy link
Collaborator

Shit. That join thing is bad, I'd like to help you sorting it out. I'm giving you committer action in the LevelUp fashion. As I get some time I'll add the appropriate contribute file.

I think one problem is in the QueryOptimizer: I am leveraging LevelDOWN's approximateSize, while it cannot be possible in the browser (https://github.com/mcollina/node-levelgraph/blob/master/lib/queryplanner.js#L19-L26). However, you can fake it very nicely by looking at the amount of variables in the query.
I can do it, if you need it.

Could you please add in the README the instructions for running the testling tests and making the browserify build?

(This browserify thing is getting bigger and bigger, I'm sorry about that).

@mcollina mcollina reopened this Jun 28, 2013
@mcollina
Copy link
Collaborator

@mcollina
Copy link
Collaborator

If you push this in a separate branch anche make another pull-request, I can code there too!

@refset
Copy link
Collaborator Author

refset commented Jun 29, 2013

So making approximateSize return 0 really was too good to be true! Ah well. I wonder if faking it is something that can and ought to live within level.js

Thank you for the repo access :) I'll be sure to practice with git some more and rebase all these commits before doing a new branch PR. Have a good weekend!

@mcollina
Copy link
Collaborator

Could you please make a new Pull-Request from this repo's 'browser' branch to this repo? There, summarize what you have done and what it is missing.
Thanks again, this work is very needed!

@refset
Copy link
Collaborator Author

refset commented Jun 30, 2013

Just a couple of questions first:

@mcollina
Copy link
Collaborator

For the time being, just use your versions. However this is not going to get merged in until they accept yours pull-requests.
However, we have things to do first (making this work in the Browser :P).

A /build/bundle.js will surely be welcomed!

@mcollina
Copy link
Collaborator

However, this is not needed: refset/level.js@6e1d9b1.

As I said, you can totally get around without it: put that call behind an if.

@mcollina
Copy link
Collaborator

mcollina commented Jul 1, 2013

Closing this in favor of #23.

@mcollina mcollina closed this Jul 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants