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

namespace all functions #67

Merged
merged 35 commits into from Sep 18, 2017
Merged

namespace all functions #67

merged 35 commits into from Sep 18, 2017

Conversation

jhoblitt
Copy link
Member

No description provided.

@jhoblitt jhoblitt force-pushed the tickets/DM-11904-namespace branch 4 times, most recently from 08ff2d9 to 169c7ce Compare September 15, 2017 18:55
@jhoblitt
Copy link
Member Author

Minor fixes/corrections were made that came up while writing examples but no major refactoring, even where it obviously should be done, was made. The intent is to establish a baseline before further changes are made.

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

The changes to newinstall.sh are fine. There is a huge amount of Ruby code here that I'm not going to review in detail. Did you auto generate this? I am worried that some of it seems to be testing things like "does the message contain the right string" which is not really what test frameworks are meant to test. For example the tests will need fixing when #68 merges (which will be before this merges because I'm waiting for Travis).

@jhoblitt
Copy link
Member Author

I'm afraid I keyed it all by hand. In many cases, there are no side effects to observe (that is, rspec-bash is able to observe) from a function to indicate which code path it has gone down other than output. It does end up being a bit more fragile for refactoring than a BDD ideal. However, I wouldn't want to try to write aruba or cucumber tests for the output of newinstall.sh, so breaking it up into small pieces is more tractable. It also means there isn't an acceptance test framework that is brittle for output changes.

@jhoblitt
Copy link
Member Author

Repushed with the missing require statement.

@jhoblitt
Copy link
Member Author

The travis env is, amazingly, missing /usr/bin/true.

@jhoblitt jhoblitt changed the title namespace most functions namespace all functions Sep 18, 2017
The current travis env does not include /usr/bin/true.
@jhoblitt jhoblitt merged commit 039a25d into master Sep 18, 2017
@ktlim ktlim deleted the tickets/DM-11904-namespace branch August 25, 2018 06:15
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.

None yet

2 participants