Skip to content

Conversation

@kaspermarkus
Copy link
Member

Temporary fall-back context handling implementation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e.

@kaspermarkus
Copy link
Member Author

@amb26 Ready for a new round of review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

illuminance, not illuminosity or luminosity

@kaspermarkus
Copy link
Member Author

@amb26 Ok, ready again

… Fixed a few other minor issues pointed out by Mr. Antvanique
…irective of jshint was taken into account. Fixed new jshint errors
…irective of jshint was taken into account. Fixed new jshint errors
@kaspermarkus
Copy link
Member Author

@amb26 Added more complete tests of the context integration, should be ready for another round of review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I remember writing this : P

@kaspermarkus
Copy link
Member Author

OK - It should be ready again now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests fail for me when run from their own directory (or indeed from all-tests.js) I believe because of this hard-coded relative path. The failure is

Error: ENOENT, no such file or directory 'E:\source\gits\gpii\node_modules\universal\tests\tests\platform\linux\configs\linux-builtIn-config.json'

I think that relative paths by default are interpreted relative to the process working directory. You should follow the pattern in DevelopmentTests.js, annoying though it is, and explicitly resolve a path with respect to __dirname which is guaranteed to be the directory holding the module file itself.

@kaspermarkus
Copy link
Member Author

Fixed path issue - ready for testing again

@kaspermarkus
Copy link
Member Author

oh, @simonbates I'm so gonna win the race

…failure point for further investigation

Merge branch 'master' into GPII-795

Conflicts:
	gpii/node_modules/contextManager/src/ContextManager.js
	gpii/node_modules/contextManager/test/ContextManagerTests.js
	tests/all-tests.js
	tests/web/html/all-tests.html
@amb26
Copy link
Member

amb26 commented Mar 26, 2015

I've updated this branch to current master, which now has GPII-17 merged into it, into my own repo's branch.

https://github.com/amb26/universal/tree/GPII-795

Unfortunately there is a problem with the tests, in that they do not actually verify that the context manager does anything on a context change - I have inserted the following failure line into the context manager impl, and all the tests still pass:

https://github.com/amb26/universal/blob/GPII-795/gpii/node_modules/contextManager/src/ContextManager.js#L171

Could you

i) Investigate and correct the implementation of the test cases so that the test fails as it should, and then
ii) Investigate and correct the context manager implementation so that when this line is removed, the tests then pass :)

Cheers

…cking the settings they should as pointed out by Mr. AnvaniQue
@kaspermarkus
Copy link
Member Author

Thanks for the update to current master and GPII-17 @amb26 .. I have fixed the issue with the tests, so things should now work properly! Thanks for catching that issue and sorry for not having done so myself.. ready for another round of review

@amb26 amb26 merged commit bef7fb5 into GPII:master Apr 8, 2015
@kaspermarkus kaspermarkus deleted the GPII-795 branch April 22, 2015 12:50
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