Skip to content

Fix parent context#54

Merged
geekdave merged 1 commit intoGeppettoJS:masterfrom
mmikeyy:patch-1
Sep 4, 2014
Merged

Fix parent context#54
geekdave merged 1 commit intoGeppettoJS:masterfrom
mmikeyy:patch-1

Conversation

@mmikeyy
Copy link
Copy Markdown
Contributor

@mmikeyy mmikeyy commented Aug 28, 2014

context.parentContext just needs to be a reference to the parent context.

createChildResolver function not required. No need to go to the parent context to do anything as the parent has no need to be aware of the child's existence (it's the child that must reference the parent).

Original createChildResolver function was calling "child" a resolver based on the parent context as this._context is the parent context when the function is called.

context.parentContext just needs to be a reference to the parent context.

createChildResolver function not required. No need to go to the parent context to do anything as the parent has no need to be aware of the child's existence (it's the child that must reference the parent).

Original createChildResolver function was calling "child" a resolver based on the parent context as this._context is the parent context when the function is called.
@geekdave
Copy link
Copy Markdown
Contributor

CI is failing. Please see https://github.com/ModelN/backbone.geppetto#pull-requests for info on how to properly add tests & run the build before submitting the PR. Feel free to let me know if you run into a snag and I can help.

@mmikeyy
Copy link
Copy Markdown
Contributor Author

mmikeyy commented Aug 28, 2014

yeah.... I just noticed that. First time I do a PR. Looking into this now. Sorry about this!

@geekdave
Copy link
Copy Markdown
Contributor

No prob! Thanks for chipping in. Hopefully the docs should tell you everything you need to know, but feel free to ask if you get stuck on anything.

@mmikeyy
Copy link
Copy Markdown
Contributor Author

mmikeyy commented Aug 29, 2014

ok. Now I am stuck.

I installed everything for beautifying, testing and all locally only to find out that it might never work after all because a paid account with saucelabs might be required for testing. Perhaps I'm wrong but I keep getting the following message:

=> Starting Tunnel to Sauce Labs
=> Sauce Labs trying to open tunnel
>> Sauce Labs Tunnel disconnected
>> Could not create tunnel to Sauce Labs
Warning: Task "saucelabs-mocha:all" failed.� Used --force, continuing.

And I read somewhere that account info must be entered in a config file for the tests to run.

I want to be able to test (I did write one and I want to write the others) but I also want to be able to test without polluting the environment (and so publicly fail!) with pull requests that are marked as not passing some test.

So what exactly is the procedure for these tests? Where can I run them (the ones I modify, not the ones I'm already failing!) to test them before submitting them?

Update: (3 hrs later...)

I now linked my fork with a Travis CI account and it seems that this might be a way to go. It does do testing, even though I haven't yet pushed any new tests to my fork... Am I on the right track?

Anyway, I'd still be interested in running these tests locally. I seem to have all it takes, only the process trips on that saucelabs-mocha tunnel problem. Anything I could do about this?

@creynders
Copy link
Copy Markdown
Contributor

Hi @mmikeyy

A word of explanation: travis is a continuous integration platform which will automatically run scripts (most of the times tests, as configured by the author of the project) whenever you issue a pull request.
This isn't related with sauce labs, which is a paid service that allows you to test your code on a plethora of browsers.

Now on to what's wrong. If you click on
screen shot 2014-08-29 at 08 29 16
it will show you what went wrong:
screen shot 2014-08-29 at 08 30 19

Granted, it's not the most helpful of warnings. One of the tasks declared in the Gruntfile is the beautification of the source code. When running the tests with grunt travis however it will not beautify the code, but rather check whether it is beautified.
So, to solve this particular error you need to run grunt beautify. You'll notice after this that backbone.geppetto.js is modified and need to add-and-commit it again.
Subsequently you can run grunt travis again.

(Hint: it will fail again, due to linting errors, fortunately these are FAR more detailed 😁 )

I know, it's a bit cumbersome and mysterious at first, but you just need to get the hang of it.

@creynders
Copy link
Copy Markdown
Contributor

Oh and normally you shouldn't get any complaints about sauce labs. At least I don't, even when run locally. Which task are you trying to run?

Correct procedure would be:

  1. do modifications, add and commit
  2. run grunt w/o arguments
  3. fix possible errors etc.
  4. repeat 2. until it no longer fails
  5. add and commit changes
  6. push branch

@geekdave
Copy link
Copy Markdown
Contributor

Yup, just run grunt with no arguments locally before pushing.

See: https://github.com/ModelN/backbone.geppetto/#build

@mmikeyy
Copy link
Copy Markdown
Contributor Author

mmikeyy commented Aug 29, 2014

wow... it works. the fancy interface that comes with my IDE seems pretty useless! Or perhaps I'm clueless...

@geekdave
Copy link
Copy Markdown
Contributor

Oh really - were you trying to use WebStorm's grunt integration? I prefer the command line myself.

@mmikeyy
Copy link
Copy Markdown
Contributor Author

mmikeyy commented Aug 29, 2014

Yep... PHPStorm actually. It baffles me that I just type 'grunt' at the command line and boom it works, while the IDE's terminal is totally unable to do anything useful, I also found that if I install all the dependencies in the right directories as required by sauce.html and index.html, I can get a superb output in the browser. I don't know though why these web pages require the very same libraries from totally different folders, forcing one to duplicate many of them. I guess one is not expected to run them in the browser (rather letting node load them?)

@mmikeyy
Copy link
Copy Markdown
Contributor Author

mmikeyy commented Aug 30, 2014

Hey! I seem to be getting the hang of it. Cool! Thanks for the tips, creynders and geekdave!

OK, now, would I be wasting my time if I finished the PR for these parent contexts by revising the tests affecting them?

I've been using them (parent contexts) quite a bit (I mean with my little patch) and I find them very useful. It's only a couple of lines of code, but I think it will involve quite a bit of work on the tests. Just don't want to do all that for nothing...

@geekdave
Copy link
Copy Markdown
Contributor

geekdave commented Sep 1, 2014

@creynders what's your take here? I haven't used parent/child contexts in my own projects, so I want to make sure the expectations are clear on this one.

@creynders
Copy link
Copy Markdown
Contributor

@mmikeyy I seem to remember Phpstorm needs some configuration before the CL works properly. I have no problems running any tool from the CL.

@geekdave yeah, me neither. I'll look into it.

creynders added a commit to creynders/backbone.geppetto that referenced this pull request Sep 4, 2014
@geekdave geekdave merged commit c8fe2b9 into GeppettoJS:master Sep 4, 2014
geekdave added a commit that referenced this pull request Sep 4, 2014
Fix tests for #54 and add missing ones for full coverage
mmikeyy pushed a commit to mmikeyy/backbone.geppetto that referenced this pull request Sep 25, 2014
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.

3 participants