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

Restore run_tests.sh #785

Merged
merged 2 commits into from
Apr 9, 2014
Merged

Conversation

oxtopus
Copy link
Contributor

@oxtopus oxtopus commented Apr 8, 2014

run_tests.sh was abruptly, and without discussion, removed from the nupic source tree, breaking internal grok pipelines, and developer workflow (see git hooks). Lack of run_tests.sh also violates numenta standard test runner specification. This PR restores it.

@rhyolight
Copy link
Member

I would rather not add this shell script back. It's easily worked around in Grok pipelines, and adds no value to the repo. Tests should be run through the new cmake interface, IMO.

@oxtopus
Copy link
Contributor Author

oxtopus commented Apr 8, 2014

I disagree. The reasoning behind having it there is that it's obvious that that's how tests are run. Feel free to modify it so it calls CMake, but there should be a single, obvious, and simple way to invoke tests.

@david-ragazzi
Copy link
Contributor

was abruptly, and without discussion

Sorry enter discussion... But the PR #751 remained for 12 days! Without take in account older related PRs! I think was enough time to discuss it... Furthermore, all were invited to review..

@breznak
Copy link
Member

breznak commented Apr 8, 2014

alternatives are:
python $NTA/bin/run_tests.py (from any path)
or
make tests_run(_all) from dir with Makefile

I agree on quick revert and discussion on ML if needed, but I'd also be
happy to see it go and use one of the alternative methods to do the same.
(run_tests.sh is anyway just a wrapper to .py)

On Wed, Apr 9, 2014 at 12:34 AM, Matthew Taylor notifications@github.comwrote:

I would rather not add this shell script back. It's easily worked around
in Grok pipelines, and adds no value to the repo. Tests should be run
through the new cmake interface, IMO.


Reply to this email directly or view it on GitHubhttps://github.com//pull/785#issuecomment-39910123
.

Marek Otahal :o)

@david-ragazzi
Copy link
Contributor

make tests_run(_all) from dir with Makefile

There's not easier way to invoke tests than this command...

@oxtopus
Copy link
Contributor Author

oxtopus commented Apr 8, 2014

run_tests.sh -h prints a meaningful help message with available options. It's not clear to me how to do the same with make tests_run. From that perspective, I don't see how removing run_tests.sh made it easier to run tests.

@david-ragazzi
Copy link
Contributor

It's make tests_run, not cmake tests_run...

@oxtopus
Copy link
Contributor Author

oxtopus commented Apr 8, 2014

Thanks, @david-ragazzi, I updated my comment, but that doesn't change the fact that make tests_run is less usable than run_tests.sh.

@rhyolight
Copy link
Member

Been thinking about this, and although I don't want this script in NuPIC, @oxtopus asked for run_tests.sh to be left in a long time ago, and we removed it without a discussion on the mailing list. We can't expect everyone to pay attention to all the Github PR and issue discussions. I think it is a legitimate request to put it back in and open up the discussion for removal on the ML. I'll merge this PR once the build passes and create a new topic on the nupic-hackers ML.

rhyolight added a commit that referenced this pull request Apr 9, 2014
@rhyolight rhyolight merged commit 2a693ed into numenta:master Apr 9, 2014
@oxtopus
Copy link
Contributor Author

oxtopus commented Apr 9, 2014

Thanks!

rhyolight added a commit to breznak/nupic that referenced this pull request Apr 15, 2014
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

4 participants