Added tests for utils/svn #2950

mansimarkaur · 2017-07-26T20:21:22Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew tests with your changes locally?

reitermarkus · 2017-07-26T21:36:39Z

Library/Homebrew/test/utils/svn_spec.rb

You shouldn't use instance variables to describe a test, since this doesn't really explain what it is supposed to do.

MikeMcQuaid · 2017-07-27T14:32:29Z

Some test failures:

Failures:
  1) Utils#self.svn_remote_exists returns true when svn is not available
     Failure/Error: expect(described_class.svn_remote_exists(url)).to be_truthy
       expected: truthy value
            got: false
     # ./test/utils/svn_spec.rb:26:in `block (3 levels) in <top (required)>'
     # ./test/spec_helper.rb:91:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.0.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'
  2) Utils#self.svn_available? returns false if svn --version command does not succeed
     Failure/Error: expect(described_class.svn_available?).to be_falsey
       expected: falsey value
            got: true
     # ./test/utils/svn_spec.rb:12:in `block (3 levels) in <top (required)>'
     # ./test/spec_helper.rb:91:in `block (2 levels) in <top (required)>'
     # ./vendor/bundle/ruby/2.0.0/gems/rspec-wait-0.0.9/lib/rspec/wait.rb:46:in `block (2 levels) in <top (required)>'
Finished in 7 minutes 2 seconds (files took 4.75 seconds to load)
985 examples, 2 failures
Failed examples:
rspec ./test/utils/svn_spec.rb:24 # Utils#self.svn_remote_exists returns true when svn is not available
rspec ./test/utils/svn_spec.rb:10 # Utils#self.svn_available? returns false if svn --version command does not succeed

mistydemeo · 2017-07-27T23:32:54Z

Library/Homebrew/test/utils/svn_spec.rb

Is this test performing network access?

Yes, it is performing network access. Do you think I should add a local repo instead?

As we discussed on Slack - preferably, it'd be good to use a local repo here. If something has to use the network and we don't have another option, it'll need the :needs_network tag so that it only gets run with brew tests --online.

The tests are behaving weirdly on my system. In the first run, only 1 test case fails, in the second 3 tests cases fail. I can kind of see this pattern here. Could you please try running the tests on your system and let me know what happens, @mistydemeo ?

mansimarkaur · 2017-07-29T02:29:33Z

@apjanke Why was this PR closed?

apjanke · 2017-07-29T04:32:03Z

I have no idea! I think I accidentally hit the wrong button on GitHub. Please ignore.

MikeMcQuaid · 2017-08-05T15:44:10Z

Library/Homebrew/test/utils/svn_spec.rb

Shouldn't need mktmpdir here; this is already in a temporary directory.

MikeMcQuaid · 2017-08-05T15:44:25Z

Library/Homebrew/test/utils/svn_spec.rb

This won't be run if the expectation fails.

I've added a begin ... ensure block to take care of this now.

reitermarkus · 2017-08-05T16:49:34Z

Library/Homebrew/test/utils/svn_spec.rb

This will not reset the variable after the test, can you find a way to test the functionality that actually sets this to true? Otherwise, the only thing this is testing is whether svn_available? uses this variable.

Or alternatively: call a method that calls this so it's covered but not by a unit test.

This variable will be set only if svn_available? has been called successfully atleast once. So, there is no method which can ensure @svn will already exist if svn is not available on the system.
I'm removing @svn before each test now. I don't think svn_version already set can work without setting @svn because on a system without svn, this test won't work the way it should.

MikeMcQuaid · 2017-08-10T13:35:12Z

Library/Homebrew/test/utils/svn_spec.rb

I think it may be worth allowing the svn shim to be called. What fails if it is?

MikeMcQuaid · 2017-08-10T13:35:14Z

Library/Homebrew/test/utils/svn_spec.rb

I think it may be worth allowing the svn shim to be called. What fails if it is?

On some systems it might be successful and on some it might not be. So, I can just check that quiet_system will return either true, nil or false if svn shim is called.

On any macOS system there will always be a working svn and git so let's assume that during the tests, too, thanks.

mistydemeo

Left one comment, but otherwise looks good to me.

mistydemeo · 2017-08-15T20:06:17Z

Library/Homebrew/test/utils/svn_spec.rb

Isn't this test just doing the same thing as the previous one?

True. I guess I should remove it. I've removed such similar tests in git tests as well. Thanks :)

MikeMcQuaid · 2017-08-23T17:55:03Z

Library/Homebrew/test/utils/svn_spec.rb

Need to try and figure out a way to not set/get instance variables here. If need be: delete the relevant tests that need them set given we're almost at the end of GSoC. Ideally: just call other methods that end up calling these ones.

Here, I'm trying to remove the instance variable if already set because once svn_available? is called, it will set the @svn instance variable. To make these tests independent of each other, I need to ensure that the instance variable is not set beforehand. What do you suggest in this case?

Perhaps adding a new method clear_svn_location_cache or similar and call that in the before.

Thanks for the suggestion! :)

MikeMcQuaid · 2017-08-23T17:57:04Z

Library/Homebrew/test/utils/svn_spec.rb

Why does this need to use mktmpdir and not write to an already existing location that's deleted after tests: for example:

brew/Library/Homebrew/test/spec_helper.rb

Lines 110 to 129 in 2c43e06

FileUtils.rm_rf [

TEST_DIRECTORIES.map(&:children),

HOMEBREW_LINKED_KEGS,

HOMEBREW_PINNED_KEGS,

HOMEBREW_PREFIX/".git",

HOMEBREW_PREFIX/"bin",

HOMEBREW_PREFIX/"share",

HOMEBREW_PREFIX/"opt",

HOMEBREW_PREFIX/"Caskroom",

HOMEBREW_LIBRARY/"Taps/caskroom",

HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bar",

HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bundle",

HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-foo",

HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-services",

HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-shallow",

HOMEBREW_REPOSITORY/".git",

CoreTap.instance.path/".git",

CoreTap.instance.alias_dir,

CoreTap.instance.path/"formula_renames.json",

]

It's also really confusing to have a directory named url. Instead just use HOMEBREW_CACHE and no let.

Got it. It's better to use a location that is already deleted after tests. Thanks!

MikeMcQuaid · 2017-08-23T17:59:19Z

Library/Homebrew/test/utils/svn_spec.rb

Consider using https://github.com/Homebrew/install as it's really small and we control it (you can clone GitHub repositories as Subversion repositories).

Using the above url, I'm getting the following:

Error validating server certificate for 'https://github.com:443': - The certificate is not issued by a trusted authority. Use the fingerprint to validate the certificate manually! Certificate information: - Hostname: github.com - Valid: from Mar 10 00:00:00 2016 GMT until May 17 12:00:00 2018 GMT - Issuer: DigiCert SHA2 Extended Validation Server CA, www.digicert.com, DigiCert Inc, US - Fingerprint: D7:9F:07:61:10:B3:92:93:E3:49:AC:89:84:5B:03:80:C1:9E:2F:8B (R)eject, accept (t)emporarily or accept (p)ermanently?

In the tests, how do I ensure that I don't need to manually answer the above posed question of accepting the host?

MikeMcQuaid · 2017-08-23T17:59:46Z

Library/Homebrew/test/utils/svn_spec.rb

HOMEBREW_CACHE.cd { system ... }

reitermarkus · 2017-08-24T05:34:45Z

Library/Homebrew/utils/svn.rb

This is never needed, except in tests, so this shouldn‘t exist in the actual code.

@reitermarkus I'm not sure how this gets resolved without adding a method here. What would you propose?

How about adding a method in one of the support modules for tests to clear any instance variable? As in a method whose argument is the instance variable to be cleared. @MikeMcQuaid @reitermarkus @mistydemeo

@mansimarkaur If by support modules you mean in the spec_helper or something then: yeh, that could work. @reitermarkus?

Also, there's probably some times this method should be called anyway such as e.g. after a new installation of svn (and same thing for git)

@MikeMcQuaid Yes, I meant adding a method in the spec_helper but if we want to call this cache clear method after a new installation, then we don't need to add it in the spec_helper. Which is a better alternative? Also, utils/git.rb already has a Utils.clear_git_available_cache to remove @git, @git_path and @git_version: https://github.com/Homebrew/brew/blob/master/Library/Homebrew/utils/git.rb#L66

@mansimarkaur I think this is fine as-is, then, thanks.

mansimarkaur · 2017-08-25T11:39:20Z

There is something wrong with codecov. The base coverage is missing. Could you please help?

mistydemeo

This looks good to me. Could you get the commit history ready for merging?

Requested changes have been made

MikeMcQuaid

One tweak required otherwise 👍

MikeMcQuaid · 2017-08-29T09:54:13Z

Library/Homebrew/dev-cmd/audit.rb

Actually: this shouldn't be a problem because it's not an issue with the formula that's being audited. Instead I'd just do next unless Utils.svn_available?

Got it! Made the change. Thanks :)

mistydemeo · 2017-08-29T22:08:09Z

Thanks!

MikeMcQuaid · 2017-08-30T09:45:08Z

Thanks for this @mansimarkaur, nice work!

mansimarkaur force-pushed the svn branch from b5ec095 to 5516c81 Compare July 26, 2017 20:30

reitermarkus requested changes Jul 26, 2017

View reviewed changes

mistydemeo reviewed Jul 27, 2017

View reviewed changes

apjanke closed this Jul 28, 2017

apjanke reopened this Jul 28, 2017

mansimarkaur force-pushed the svn branch from 06bced4 to a4cbe49 Compare August 5, 2017 15:16

MikeMcQuaid requested changes Aug 5, 2017

View reviewed changes

reitermarkus requested changes Aug 5, 2017

View reviewed changes

MikeMcQuaid requested changes Aug 10, 2017

View reviewed changes

mansimarkaur force-pushed the svn branch 2 times, most recently from 5fb5025 to f659a03 Compare August 15, 2017 11:20

mistydemeo approved these changes Aug 15, 2017

View reviewed changes

mistydemeo approved these changes Aug 18, 2017

View reviewed changes

MikeMcQuaid previously requested changes Aug 23, 2017

View reviewed changes

mansimarkaur force-pushed the svn branch from 28f5b5f to 07bd34f Compare August 23, 2017 18:21

reitermarkus reviewed Aug 24, 2017

View reviewed changes

mistydemeo approved these changes Aug 26, 2017

View reviewed changes

mansimarkaur force-pushed the svn branch 2 times, most recently from 97565f3 to dfe7338 Compare August 26, 2017 20:07

MikeMcQuaid approved these changes Aug 29, 2017

View reviewed changes

mansimarkaur force-pushed the svn branch from dfe7338 to 3d8873c Compare August 29, 2017 10:41

mansimarkaur added 3 commits August 29, 2017 16:16

Added tests fir utils/svn

13fb14a

Improved tests for svn_available?

cf96d8d

Added check for svn availability

53be6bb

mansimarkaur added 3 commits August 29, 2017 16:16

Added clear_svn_version_cache

d9c0b8c

Used clear_svn_version_cache to remove @svn

d533973

url skipped if svn not available when auditing urls

3d8873c

reitermarkus approved these changes Aug 29, 2017

View reviewed changes

mistydemeo merged commit 156bca7 into Homebrew:master Aug 29, 2017

Homebrew locked and limited conversation to collaborators May 4, 2018

	FileUtils.rm_rf [
	TEST_DIRECTORIES.map(&:children),
	HOMEBREW_LINKED_KEGS,
	HOMEBREW_PINNED_KEGS,
	HOMEBREW_PREFIX/".git",
	HOMEBREW_PREFIX/"bin",
	HOMEBREW_PREFIX/"share",
	HOMEBREW_PREFIX/"opt",
	HOMEBREW_PREFIX/"Caskroom",
	HOMEBREW_LIBRARY/"Taps/caskroom",
	HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bar",
	HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-bundle",
	HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-foo",
	HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-services",
	HOMEBREW_LIBRARY/"Taps/homebrew/homebrew-shallow",
	HOMEBREW_REPOSITORY/".git",
	CoreTap.instance.path/".git",
	CoreTap.instance.alias_dir,
	CoreTap.instance.path/"formula_renames.json",
	]

Uh oh!

Added tests for utils/svn #2950

Added tests for utils/svn #2950

Uh oh!

Conversation

mansimarkaur commented Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MikeMcQuaid commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mansimarkaur commented Jul 29, 2017

Uh oh!

apjanke commented Jul 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mansimarkaur Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mistydemeo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mansimarkaur commented Jul 26, 2017 •

edited

Loading

mansimarkaur Aug 12, 2017 •

edited

Loading

mansimarkaur Aug 25, 2017 •

edited

Loading