Skip to content
This repository has been archived by the owner on Sep 1, 2023. It is now read-only.

Pythonize the tests calls #1622

Merged
merged 4 commits into from
Dec 19, 2014
Merged

Conversation

david-ragazzi
Copy link
Contributor

This PR aims:

  • Replace run_tests.sh to a more cross platform and default approach. I.e. the run_nupic_tests executable script.
  • Replace all tests calls from make to $NUPIC/scripts/run_nupic_tests (except testpyhtm which is called from $NUPIC/bin).

It allows any user to type $NUPIC/scripts/run_nupic_tests in everywhere* on shell and to have access to all nupic tests. Example:

$ $NUPIC/scripts/run_nupic_tests --help

@rhyolight
Copy link
Member

@david-ragazzi Should there be a README change?

@david-ragazzi david-ragazzi force-pushed the tests_entry_point branch 2 times, most recently from 66a4a28 to 346756d Compare December 14, 2014 22:33
@david-ragazzi
Copy link
Contributor Author

@david-ragazzi Should there be a README change?

I updated README with a few instructions:
https://github.com/numenta/nupic/pull/1622/files#diff-04c6e90faac2675aa89e2176d2eec7d8R92

@scottpurdy
Copy link
Contributor

How does run_nupic_tests find the tests?

@david-ragazzi david-ragazzi changed the title Replace "run_tests.sh" to the cross platform "run_nupic_tests" Pythonize the tests calls Dec 15, 2014
@david-ragazzi
Copy link
Contributor Author

How does run_nupic_tests find the tests?

run_nupic_tests is simply the scripts/run_tests.py file renamed to scripts/run_nupic_tests which is installed to some bin folder. If user uses install option without --user (i.e. with sudo), this bin folder will be /usr/local/bin (which already is in $PATH on OSX and Linux by default), otherwise he/she will have to put the local bin folder to $PATH or then simply use $NUPIC/scripts/run_nupic_tests.

Update:

run_nupic_tests won't installed at moment, so users should use $NUPIC/scripts/run_nupic_tests.

@david-ragazzi
Copy link
Contributor Author

🍏

Should I update CHANGELOG with this minor change?

* Adjusted tests calls to use default features of setuptools.

@scottpurdy
Copy link
Contributor

@david-ragazzi - I understand how the executable script is discovered. My question was how the script finds the tests. Does this work both with regular installations and developer installations? Our tests are intentionally not on the Python path (for now) so I wouldn't expect them to be installed. Thus installing the script that executes them seems odd. Now that is a bit confounded because I think we might actually be installing the tests. So I can see including the test runner, but that brings up the original question of how the test runner knows where to find the tests.

@rhyolight
Copy link
Member

Should I update CHANGELOG with this minor change?

  • Adjusted tests calls to use default features of setuptools.

Yes I think so, because there's a change in the README for users to run tests.

@david-ragazzi
Copy link
Contributor Author

Does this work both with regular installations and developer installations?

It works only when install_scripts is called which usually happens when install* and develop commands are specified. So, yes, it's supposed that it works both with regular installations and developer installations.

brings up the original question of how the test runner knows where to find the tests.

Look at this:
https://github.com/david-ragazzi/nupic/blob/tests_entry_point/scripts/run_nupic_tests#L263

It simply gets the location of the tests using $NUPIC.

So I agree with you when you say our tests are intentionally not on the Python path (for now) so I wouldn't expect them to be installed. As run_nupic_tests use $NUPIC (which is set only for development purposes), doesn't make sense we install this script. Sorry for this mistake. This said, instead of we install run_nupic_tests we simply could inform users to call it using $NUPIC/scripts/run_nupic_tests which still is entry point and can be called from everywhere. What do you think?

By the way, as I said in #1564, I'm not in favor of include tests into nupic package as tests usually are for development purposes. This article illustrates well some reasons for this:

http://blog.ionelmc.ro/2014/05/25/python-packaging/

.. but let's focus on $NUPIC/scripts/run_nupic_tests at moment, and move this discussion for other place (maybe here: #1564). If we decide move tests into nupic package in future, then we update the build to install run_nupic_tests script.

@david-ragazzi
Copy link
Contributor Author

I removed the code related to install the run_nupic_tests and updated the header of this PR.

Sorry for number of commits, I'm not on my machine, so I'm updating the repo using browser.. By the way, the number of changes is quite small and easy to review.

@rhyolight
Copy link
Member

I merged in master.

@david-ragazzi david-ragazzi force-pushed the tests_entry_point branch 2 times, most recently from fa8e832 to 398d2e3 Compare December 16, 2014 20:34
@david-ragazzi
Copy link
Contributor Author

I won't update CHANGELOG until a positive review. It's creating merge conflicts to each PR that is merged on master. When this PR is ready to merge, I update it.

@david-ragazzi david-ragazzi reopened this Dec 16, 2014
@david-ragazzi david-ragazzi force-pushed the tests_entry_point branch 2 times, most recently from 13723c3 to f85908a Compare December 17, 2014 20:58
@david-ragazzi
Copy link
Contributor Author

Travis is green. Could someone review?

@rhyolight @oxtopus @scottpurdy

@rhyolight
Copy link
Member

@david-ragazzi Please update .githooks/pre-commit with the new test runner code. Other than that, LGTM.

@david-ragazzi
Copy link
Contributor Author

@david-ragazzi Please update .githooks/pre-commit with the new test runner code. Other than that, LGTM.

Done.

@rhyolight
Copy link
Member

👍 Works locally (including githook). @scottpurdy, any more questions?

@scottpurdy
Copy link
Contributor

👍

@rhyolight
Copy link
Member

@david-ragazzi I'll merge once the build passes, but builds are stalled on OS X jobs... 😕

@rhyolight
Copy link
Member

@david-ragazzi I did a bunch of work yesterday, including breaking builds. This merge from master should fix it. Will merge once it passes.

rhyolight added a commit that referenced this pull request Dec 19, 2014
@rhyolight rhyolight merged commit 30fecdd into numenta:master Dec 19, 2014
@david-ragazzi
Copy link
Contributor Author

Thanks! Although I see Travis was red....

@rhyolight
Copy link
Member

It passed before I updated the changelog

Sent from my MegaPhone

On Dec 19, 2014, at 6:11 PM, David Ragazzi notifications@github.com wrote:

Thanks! Although I see Travis was red....


Reply to this email directly or view it on GitHub.

@david-ragazzi david-ragazzi deleted the tests_entry_point branch December 21, 2014 13:20
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.

3 participants