Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Use '+' as single level wildcard to make it consistent with mqtt ascoltatore #62

Merged
merged 17 commits into from Jun 28, 2013
Merged

Use '+' as single level wildcard to make it consistent with mqtt ascoltatore #62

merged 17 commits into from Jun 28, 2013

Conversation

davedoesdev
Copy link
Collaborator

No description provided.

@mcollina
Copy link
Collaborator

Could you please add a test for it?

@mcollina
Copy link
Collaborator

Add it in the behave_as_ascoltatore.
We should enforce more tight test for the wildcards.

As the MemoryAscoltatori cannot be made to pass these easily, feel free to drop it entirely: trie rocks.

@davedoesdev
Copy link
Collaborator Author

Should be done with that commit, was more work than I thought it would be.

@mcollina
Copy link
Collaborator

The build is failing :(

@mcollina
Copy link
Collaborator

@davedoesdev
Copy link
Collaborator Author

Strange - builds here. I'll look into it, it's the MQTT test which is failing.

@davedoesdev
Copy link
Collaborator Author

Couple of problems I think:

  1. Mosca still depends on Ascoltatori 0.6
  2. Ascoltatori is still giving out MemoryAscoltatore as default

@mcollina
Copy link
Collaborator

I will try releasing a new version of Mosca this evening or tomorrow. I am closing moscajs/mosca#36.

@davedoesdev davedoesdev mentioned this pull request Jun 26, 2013
9 tasks
@davedoesdev
Copy link
Collaborator Author

Fixed making TrieAscoltatore default - it was missed in ascoltatori.build

Although it fails on Travis, the tests now pass for me on my local machine with mosca (persistence branch) using the version of Ascoltatori on this pull request. That's because Mosca on Travis won't have these changes.

@mcollina
Copy link
Collaborator

I have published the persistence branch, it is released as 0.7.2.
However, I have restarted the build and it still fails.

@davedoesdev
Copy link
Collaborator Author

Seems like mosca is pulling in ascoltatori 0.7.4 instead of the version on this branch:

mosca@0.7.0 node_modules/mosca
├── lru-cache@2.3.0
├── tmp@0.0.20
├── commander@1.1.1 (keypress@0.1.0)
├── minimatch@0.2.12 (sigmund@1.0.0)
├── ascoltatori@0.7.4

@davedoesdev
Copy link
Collaborator Author

This is strange as on my machine I get this:

mosca@0.7.2 node_modules/mosca
├── lru-cache@2.3.0
├── tmp@0.0.20
├── level-range@0.0.0 (through@2.2.7)
├── commander@1.1.1 (keypress@0.1.0)
├── minimatch@0.2.12 (sigmund@1.0.0)
├── memdown@0.2.0 (abstract-leveldown@0.7.1)
├── bunyan@0.21.3 (mv@0.0.5, dtrace-provider@0.2.8)
├── level-ttl@0.2.0 (after@0.7.0, xtend@2.0.6)
├── levelup@0.10.0 (concat-stream@0.1.1, simple-bufferstream@0.0.2, prr@0.0.0, errno@0.0.3, semver@1.1.4, xtend@2.0.6)
├── level-sublevel@4.7.0 (level-fix-range@1.1.2, string-range@1.2.1, level-hooks@4.3.2, xtend@2.0.6)
└── leveldown@0.6.1 (bindings@1.1.0)

(and the build succeeds)

@davedoesdev
Copy link
Collaborator Author

  • Why did Travis use mosca 0.7.0 instead of 0.7.2?
  • Why didn't mosca use the ascoltatori from its parent directory instead of fetch a new one (that's what happens on my machine which is why it's succeeding)?

@mcollina
Copy link
Collaborator

It is how NPM work for Mosca to fetch a new version of it from scratch.
In the package.json of Ascoltatori we are depending on the git master of Mosca, try pinning it to 0.7.2.

However it is not normal that you have such different behavior on travis and on your machine.

@davedoesdev
Copy link
Collaborator Author

I guess that build was run when Mosca was at 0.7.0 then?

@davedoesdev
Copy link
Collaborator Author

Picks up 0.7.2 now so that's sorted.
In order to test pull requests, npm on travis should detect it's installing mosca within ascoltatori and not go grab the published version.

@davedoesdev
Copy link
Collaborator Author

Hmm. This seems relevant? https://github.com/isaacs/npm/issues/2063

@davedoesdev
Copy link
Collaborator Author

Ah I wonder if it's because I still have version at 0.7.2 in the package.json in this branch. Doesn't really explain why it does it on my machine.

@davedoesdev
Copy link
Collaborator Author

Obviously not :( Very strange
@mcollina What happens if you take this branch and do an npm install then make test?

@davedoesdev
Copy link
Collaborator Author

Ah got it
Per https://npmjs.org/doc/folders.html:

Cycles are handled using the property of node's module system that it walks up the directories looking for node_modules folders. So, at every stage, if a package is already installed in an ancestor node_modules folder, then it is not installed at the current location.

I happen to have this branch checked out in a node_modules directory! That explains the difference with travis.

@davedoesdev
Copy link
Collaborator Author

OK build is now passing after I made the change in 3e1ac2f:

+before_script:
+ - (cd node_modules/mosca/node_modules && rm -rf ascoltatori && ln -s ../../.. ascoltatori)

Is that going to be okay for all pull requests going forward? It basically says tests must pass with mosca using this version of ascoltatori.
I guess we could even run the mosca tests although that might be a step too far.

@mcollina
Copy link
Collaborator

It should be fine, this cyclic dependency between them is very tricky. I just hope your fix solves it for good!

@davedoesdev
Copy link
Collaborator Author

So I added a script to run the mosca tests too. I think you have a problem with make bail in mosca:

./node_modules/.bin/mocha --recursive test --bail --reporter spec | ./node_modules/.bin/bunyan
/bin/sh: 1: ./node_modules/.bin/mocha: not found
The command "echo \$\ \(cd\ node_modules/mosca\ \&\&\ npm\ test\)" exited with 0.

The pipe means errors in the actual test won't be reported.

I'll fix the error here by doing an npm install first but just letting you know that the mosca target for 'make bail' probably should change.

@mcollina
Copy link
Collaborator

No, we should not run the Mosca tests here.

@davedoesdev
Copy link
Collaborator Author

OK I'll revert those commit then.
Just a heads up that mosca tests are failing when run against this ascoltatori. So I think we need a way to run mosca tests against ascoltatori pull requests.

@davedoesdev
Copy link
Collaborator Author

Btw, your mosca tests on travis (which appear to have succeeded) actually failed:

✖ 1 of 214 tests failed:

  1. mosca.persistence.Mongo "before each" hook:
    MongoError: ns not found
    at Object.exports.toError (/home/travis/build/mcollina/mosca/node_modules/mongodb/lib/mongodb/utils.js:108:11)
    at Db.dropCollection (/home/travis/build/mcollina/mosca/node_modules/mongodb/lib/mongodb/db.js:929:33)
    at Db._executeQueryCommand (/home/travis/build/mcollina/mosca/node_modules/mongodb/lib/mongodb/db.js:1645:20)
    at g (events.js:192:14)
    at EventEmitter.emit (events.js:126:20)
    at Server.Base._callHandler

(https://travis-ci.org/mcollina/mosca/jobs/8471162)

That 'make bail' pipe is causing failures to be missed.

@davedoesdev
Copy link
Collaborator Author

(if you want to carry on using the pipe, you could use $PIPESTATUS to make sure the errors from mocha are propagated)

@mcollina
Copy link
Collaborator

I just saw that. Gosh. It is caused by bunyan, I'll remove it ASAP.

If Mosca's tests are failing but Ascoltatori's tests are fine we have a huge problem.
As Ascoltatori's suite is very tight, we should not encounter that.

@mcollina mcollina mentioned this pull request Jun 27, 2013
7 tasks
@mcollina
Copy link
Collaborator

I have just released Mosca 0.7.3 that solves all these issues.

@davedoesdev
Copy link
Collaborator Author

OK I reverted the changes which ran the mosca tests too.
Build seems fine on Travis.

mcollina added a commit that referenced this pull request Jun 28, 2013
Use '+' as single level wildcard to make it consistent with MQTT.
@mcollina mcollina merged commit 1f84561 into moscajs:master Jun 28, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants