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

Remove what Command From Integration Tests #73

Merged
merged 4 commits into from
Mar 27, 2020
Merged

Remove what Command From Integration Tests #73

merged 4 commits into from
Mar 27, 2020

Conversation

H0R5E
Copy link
Member

@H0R5E H0R5E commented Mar 26, 2020

This commit replaces the use of the what command for finding the root directory of the source code with the function WecOptLib.utils.getSrcRootPath.

getSrcRootPath is also updated to generate a nicer looking result.

Test results (with two installations of WecOptLib):

test_results.pdf

This commit replaces the use of the what command for finding
the root directory of the source code with the function
WecOptLib.utils.getSrcRootPath.

getSrcRootPath is also updated to generate a nicer
looking result.
@H0R5E H0R5E requested a review from ssolson March 26, 2020 16:56
@H0R5E H0R5E mentioned this pull request Mar 26, 2020
7 tasks
@H0R5E
Copy link
Member Author

H0R5E commented Mar 26, 2020

Oh, b*lls. It ran the older library for the tests. Hang on, I'll prove it works....

@H0R5E
Copy link
Member Author

H0R5E commented Mar 26, 2020

Yeah, having two installs of the toolbox is weird, but here you go:

test_results.pdf

@ssolson
Copy link
Collaborator

ssolson commented Mar 26, 2020

Hey Mat there are issues using cd in Linux (and probably MacOS?) I'll send you the errors when the test finish

@ssolson
Copy link
Collaborator

ssolson commented Mar 26, 2020

Linux failing on cd

test_results.pdf

@H0R5E
Copy link
Member Author

H0R5E commented Mar 26, 2020

Sterling what does WecOptLib.utils.getSrcRootPath() give you?

@ssolson
Copy link
Collaborator

ssolson commented Mar 26, 2020

K>> WecOptLib.utils.getSrcRootPath()

ans =

    'gpfs1/ssolson/WecOptTool'

@H0R5E
Copy link
Member Author

H0R5E commented Mar 26, 2020

OK, the path was not absolute on linux. Give that a try.

@ryancoe
Copy link
Member

ryancoe commented Mar 26, 2020

can we also start using fullfile(fpath, name) instead of fp = [path filesep fname]; throughout the whole tool?

@H0R5E
Copy link
Member Author

H0R5E commented Mar 26, 2020

Yes, I think that's a good idea, but I'm not going to do it now.

@ssolson
Copy link
Collaborator

ssolson commented Mar 26, 2020

All pass Linux
test_results.pdf

@H0R5E
Copy link
Member Author

H0R5E commented Mar 26, 2020

Alright, I'll use this in the new release candidate.

@ssolson
Copy link
Collaborator

ssolson commented Mar 26, 2020

All tests pass on Windows with multiple WecOptTools. But here is something interesting. I ran the test from Documents/WecOptTool but it saved the tests in Desktop/alphaWOT/WecOptTool.

This definitely does not work as expected with multiple installations, but we can worry about this in the future.

image

test_results.pdf

@H0R5E
Copy link
Member Author

H0R5E commented Mar 26, 2020

I mean I think if you install it twice you should expect some strange results!

@H0R5E
Copy link
Member Author

H0R5E commented Mar 26, 2020

We should add a note in the docs to uninstall first if using the stable version to avoid this fun happening to folks.

@ssolson
Copy link
Collaborator

ssolson commented Mar 26, 2020

Good point. I don't know how to "uninstall".

* Add note about removal of existing installations
* Fix NEMOH Files section for current process
@H0R5E
Copy link
Member Author

H0R5E commented Mar 27, 2020

I added an update to the README for users to uninstall if they have existing versions.

@H0R5E H0R5E merged commit 28380a1 into SNL-WaterPower:master Mar 27, 2020
@H0R5E H0R5E deleted the remove_what branch March 27, 2020 09:18
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