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

GITDIST_MOVE_TO_BASE_DIR Documentation and Testing #237

Merged
merged 6 commits into from
Nov 3, 2017

Conversation

jmgate
Copy link
Collaborator

@jmgate jmgate commented Nov 2, 2017

This PR is to contain the documentation and unit testing of the GITDIST_MOVE_TO_BASE_DIR feature we created in #203.

Closes #212.

@jmgate
Copy link
Collaborator Author

jmgate commented Nov 2, 2017

So far this just has changes to the unit test suite such that all tests pass if you have GITDIST_MOVE_TO_BASE_DIR set to something before running the test suite. Working on unit testing now…

@jmgate jmgate changed the title GITDIST_MOVE_TO_BASE_DIR Documentation and Testing [WIP] GITDIST_MOVE_TO_BASE_DIR Documentation and Testing Nov 2, 2017
@bartlettroscoe
Copy link
Member

@jmgate, thanks! There was one use case in particular that caused gitdist to hang when you you set the env var.

However, I don't see any documentation added for this. Should there be a --dist-help=move-to-base-dir output option added for this? I don't see any modifications to the gitdist.py script itself.

@jmgate
Copy link
Collaborator Author

jmgate commented Nov 2, 2017

Yup, I'm working on it right now. Pushing soon…

@jmgate
Copy link
Collaborator Author

jmgate commented Nov 2, 2017

Tests in place—adding documentation now…

@jmgate jmgate force-pushed the 212-moveToBaseDir branch from c7cee59 to 0f4c14f Compare November 2, 2017 20:49
The ability to use the environment variable GITDIST_MOVE_TO_BASE_DIR to
change gitdist's behavior is now documented (see
--dist-help=move-to-base-dir) and tested (see the
gitdist_move_to_base_dir unit test).
@jmgate jmgate force-pushed the 212-moveToBaseDir branch from 0f4c14f to c7d9665 Compare November 2, 2017 20:52
@jmgate jmgate changed the title [WIP] GITDIST_MOVE_TO_BASE_DIR Documentation and Testing GITDIST_MOVE_TO_BASE_DIR Documentation and Testing Nov 2, 2017
@jmgate
Copy link
Collaborator Author

jmgate commented Nov 2, 2017

Pretty sure this PR is good to go now as well.

Looks better formatted especially when using --dist-help=all.

Also, moved --dist-help=move-to-base-dir above 'usage-tips' and
'script-dependencies' since I think that order seems a little better,
especially for --dist-help=all.

I also had to fix the tests to work when GITDIST_MOVE_TO_BASE_DIR is not set
in the outer shell.
…e exists (TriBITSPub#203)

Currently, this unit test just causes gitdist to hang until the overall test
times out.
@bartlettroscoe
Copy link
Member

@jmgate,

Thanks for adding all of this. I think this is a nice feature to add to the script.

I added some commits to your topic branch to change the formatting of the documentation a little and update the TriBITS Developers Guide for this.

I also added a commit with a unit test for a test case that causes gitdist to hang that I have noticed. That is, if you set GITDIST_MOVE_TO_BASE_DIR=IMMEDIATE_BASE or EXTREME_BASE and then run gitdist in a git repo where there is no .gitdist[.default] file in the parent git repos, then it just hangs. The documentation says it should just run gitdist in the current working directly.

Can you take a look at this use case and see if you can fix the code so that it has the expected behavior?

@jmgate
Copy link
Collaborator Author

jmgate commented Nov 3, 2017

Hmmm, I thought I had that case handled. Yeah I'll look into it today.

@jmgate
Copy link
Collaborator Author

jmgate commented Nov 3, 2017

Ah, looks like it works as documented for the default or EXTREME_BASE, but not for IMMEDIATE_BASE. Let me check the implementation real quick.

IMMEDIATE_BASE now behaves as documented in the absence of any
.gitdist[.default] file in the directory tree.
@jmgate
Copy link
Collaborator Author

jmgate commented Nov 3, 2017

@bartlettroscoe, I updated the implementation for IMMEDIATE_BASE and now I'm seeing the expected behavior for all cases. Can you confirm?

Also added a unit test for when GITDIST_MOVE_TO_BASE_DIR is set to some
invalid value.
@jmgate
Copy link
Collaborator Author

jmgate commented Nov 3, 2017

Also added a unit test for when GITDIST_MOVE_TO_BASE_DIR is set to something invalid. I think these changes are good to go again.

@bartlettroscoe
Copy link
Member

Looks great. I am merging and pushing now with checkin-test.py script for TriBITS.

GITDIST_MOVE_TO_BASE_DIR=IMMEDIATE_BASE will be my new default.

@bartlettroscoe bartlettroscoe merged commit 86a4a1d into TriBITSPub:master Nov 3, 2017
@jmgate jmgate deleted the 212-moveToBaseDir branch November 6, 2017 15:19
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.

2 participants