Skip to content

Do not allow changes to ENV to leak from test to test #403

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 17, 2019
Merged

Do not allow changes to ENV to leak from test to test #403

merged 1 commit into from
Feb 17, 2019

Conversation

jcouball
Copy link
Member

@jcouball jcouball commented Jan 30, 2019 •
edited
Loading

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

  • Ensure all commits include DCO sign-off.
  • Ensure that your contributions pass unit testing.
  • Ensure that your contributions contain documentation if applicable.

Description

Currently, the TestThreadSafety test case occasionally fails when executed as part of the build with this error:

#<Thread:0x00007fd08ea136f8@/Users/couballj/SynologyDrive/Documents/Projects/ruby-git-master-2/tests/units/test_thread_safty.rb:19 run> terminated with exception (report_on_exception is true):
Traceback (most recent call last):
	4: from /Users/couballj/SynologyDrive/Documents/Projects/ruby-git-master-2/tests/units/test_thread_safty.rb:20:in `block (2 levels) in test_git_init_bare'
	3: from /Users/couballj/SynologyDrive/Documents/Projects/ruby-git-master-2/lib/git.rb:140:in `init'
	2: from /Users/couballj/SynologyDrive/Documents/Projects/ruby-git-master-2/lib/git/base.rb:67:in `init'
	1: from /Users/couballj/SynologyDrive/Documents/Projects/ruby-git-master-2/lib/git/lib.rb:40:in `init'
/Users/couballj/SynologyDrive/Documents/Projects/ruby-git-master-2/lib/git/lib.rb:983:in `command': git '--git-dir=/var/folders/xv/2j9g0mgs5vgcdrl0s9pn9l_40000gn/T/d20190129-61742-zetuf4/.git' init '--bare'  2>&1:fatal: GIT_WORK_TREE (or --work-tree=<directory>) not allowed without specifying GIT_DIR (or --git-dir=<directory>) (Git::GitExecuteError)

the important part is:

git '--git-dir=/var/folders/xv/2j9g0mgs5vgcdrl0s9pn9l_40000gn/T/d20190129-61742-zetuf4/.git' init '--bare'  2>&1:fatal: GIT_WORK_TREE (or --work-tree=<directory>) not allowed without specifying GIT_DIR (or --git-dir=<directory>)

It can be made to fail consistently by greatly increasing the thread count. 50 threads on my test machine makes if fail every time.

I found that other tests alter the ENV global variable which makes the test_thread_safety test suite fail.

This change makes it so that tests always restore ENV to its original state.

I think this is a short term fix. A better fix would be to pass the desired environment into any method that creates a Git::Base (defaulting to a copy of ENV) and not use the ENV global through out the code.

Signed-off-by: James Couball jcouball@yahoo.com

@jcouball
Do not allow changes to ENV to leak from test to test
ae1be05 
Signed-off-by: James Couball <jcouball@yahoo.com>
@jcouball
Copy link
Member Author

jcouball commented Jan 30, 2019 •
edited
Loading

I believe this fixes issues #319 and #358 as well as the build issue reported in PRs #400 and #404

@jcouball
Copy link
Member Author

@perlun and @tarcinil -- this PR is ready for review.

@jcouball jcouball mentioned this pull request Feb 5, 2019
Closed
3 tasks
perlun
perlun approved these changes Feb 17, 2019
View reviewed changes
Copy link
Contributor

@perlun perlun left a comment

Choose a reason for hiding this comment

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

Looks good, bonus points for fixing the file with an obvious typo in the file name! 👍

tests/test_helper.rb
ensure
Git::Lib::ENV_VARIABLE_NAMES.each { |k| ENV[k] = saved_env[k] }
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

(This reminds me about my dislike for test-unit; with rspec, this could easily be done using an around {} block instead.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer rspec for the structure it makes possible. I suspect that others don't care for rspec because that structure comes with additional complexity and a steeper learning curve.

That said, I respect any project that has tests no matter what framework!

Copy link
Contributor

Choose a reason for hiding this comment

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

Comparing rspec to other test frameworks in general, like testng in Java, I think the fact that you can actually "group" and structure your tests with rspec is a big boon. It's one of the things I'm missing from no longer using it actually.

If you feel like rewriting the tests to use rspec, I clearly wouldn't object, but this is definitely not an obligation in any way. 😃

@perlun perlun merged commit 9bd4407 into ruby-git:master Feb 17, 2019
AgoraSecurity pushed a commit to AgoraSecurity/ruby-git that referenced this pull request Feb 21, 2020
@jcouball @AgoraSecurity
Do not allow changes to ENV to leak from test to test (ruby-git#403)
1c38db5 
Signed-off-by: James Couball <jcouball@yahoo.com>Signed-off-by: Agora Security <github@agora-security.com>
This was referenced Apr 25, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@perlun perlun perlun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jcouball @perlun