Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pass down stdin in 'import' check for extensions during sanity check #2276

Merged

Conversation

boegel
Copy link
Member

@boegel boegel commented Jul 13, 2017

By not passing down stdin into run_cmd in the sanity_check implementation for extensions, the 'import' check for R libraries is basically being skipped...

In addition, the caching mechanism for run_cmd should also take into account the inp argument.

@boegel
Copy link
Member Author

boegel commented Jul 13, 2017

This probably needs enhanced tests to verify the changes (and make sure the bug being fixed here isn't re-introduced).

@boegel
Copy link
Member Author

boegel commented Jul 13, 2017

Without the changes to the Seurat easyconfig in easybuilders/easybuild-easyconfigs#4889, the sanity check fails as it should on top of this...

@boegel
Copy link
Member Author

boegel commented Aug 7, 2017

tests are enhanced to catch the bug

@easybuilders/easybuild-framework-maintainers ready for review

ocaisa
ocaisa previously approved these changes Aug 25, 2017
Copy link
Member

@ocaisa ocaisa left a comment

Choose a reason for hiding this comment

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

@@ -118,6 +118,22 @@ def test_run_cmd_simple(self):
self.assertEqual(True, run_cmd("echo hello", simple=True))
self.assertEqual(False, run_cmd("exit 1", simple=True, log_all=False, log_ok=False))

def test_run_cmd_cache(self):
Copy link
Member

@ocaisa ocaisa Aug 25, 2017

Choose a reason for hiding this comment

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

What about a test for the (caching of the) new inp argument to run_cmd?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point, added in #bc4be4154

@ocaisa ocaisa merged commit 16a392b into easybuilders:develop Aug 25, 2017
@boegel boegel deleted the run_cmd_inp_extensions_sanity_check branch August 25, 2017 14:59
boegel added a commit to boegel/easybuild-framework that referenced this pull request Aug 29, 2017
wpoely86 added a commit that referenced this pull request Aug 29, 2017
fix test_inject_checksums that got broken by changes to toy-0.0-gompi-1.3.12-test.eb test easyconfig in PR #2276
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants