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

linkage_checker: fix detection of broken dependencies and missing libraries #3940

Merged
merged 13 commits into from Apr 8, 2018

Conversation

Projects
None yet
4 participants
@maxim-belkin
Copy link
Contributor

maxim-belkin commented Mar 17, 2018

  • 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 style with your changes locally?
  • Have you successfully run brew tests with your changes locally?

  1. In display_test_output: when printing dependencies without linkage, use Dependencies with no linkage string instead of Possible unnecessary dependencies for consistency with display_normal_output

  2. In check_dylibs: keep track of and skip repetitive checking of those dylibs that have been previously checked. This is applicable when different executables/libraries are linked against the same libraries.

  3. In check_undeclared_deps: when there are missing libraries, corresponding dependencies should be excluded from the list of dependencies with no linkage.

brew style linkage_checker.rb

fails with

== linkage_checker.rb ==
C: 32: 41: Use hash rockets syntax.

but this is not touched in this PR, so I'm leaving it "as is". Note, that brew style passes just fine:

$ brew style
[snip]
670 files inspected, no offenses detected

@ilovezfs

Fixes for linkage_checker
1. In "display_test_output": when printing dependencies without
linkage, use "Dependencies with no linkage" string instead of "Possible
unnecessary dependencies" for consistency with "display_normal_output"

2. In "check_dylibs": keep track of and skip repetitive checking of
those dylibs that have been previously checked. This is applicable when
different executables/libraries are linked against the same libraries.

3. In "check_undeclared_deps": when there are missing libraries,
corresponding dependencies should be excluded from the list of
dependencies with no linkage.
@@ -83,6 +86,12 @@ def check_undeclared_deps
next true if Formula[name].bin.directory?
@brewed_dylibs.keys.map { |x| x.split("/").last }.include?(name)
end
missing = []
@broken_dylibs.each do |str|
next unless str.start_with? "#{HOMEBREW_PREFIX}/opt"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 18, 2018

Member

or Cellar?

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 18, 2018

Contributor

oh, kegs... sure.

next unless str.start_with? "#{HOMEBREW_PREFIX}/opt"
missing << str.sub("#{HOMEBREW_PREFIX}/opt/", "").split("/")[0]
end
unnecessary_deps -= missing

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 18, 2018

Member

It would be good to maybe output these missing in some form?

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 18, 2018

Contributor

But missing deps can already be found with brew missing...

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 18, 2018

Contributor

I can convert broken_dylibs to Hash to make Missing libraries: output similar to that of Homebrew libraries:, that is:

$ brew linkage tor
System libraries:
  /usr/lib/libSystem.B.dylib
  /usr/lib/libz.1.dylib
Homebrew libraries:
  /usr/local/opt/libevent/lib/libevent-2.1.6.dylib (libevent)
Missing libraries:
  /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib (openssl)
  /usr/local/opt/openssl/lib/libssl.1.0.0.dylib (openssl)

Would that work?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

That seems like a good call 👍

@@ -30,6 +31,7 @@ def check_dylibs
# when checking for broken linkage
file.dynamically_linked_libraries(except: :LC_LOAD_WEAK_DYLIB).each do |dylib|
@reverse_links[dylib] << file
next if checked_dylibs.include? dylib

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 18, 2018

Member

This just eliminates checking the same dylib more than once, right?

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 18, 2018

Contributor

right. This check kicks in when different executables/libraries of the formula link to the dylib that has been already processed and added to the appropriate list.

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Mar 18, 2018

$ brew install wget
...
$ brew uninstall --ignore-dependencies openssl
...
$ brew linkage wget
System libraries:
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  /usr/lib/libSystem.B.dylib
  /usr/lib/libiconv.2.dylib
  /usr/lib/libz.1.dylib
Homebrew libraries:
  /usr/local/opt/gettext/lib/libintl.8.dylib (gettext)
  /usr/local/opt/libidn2/lib/libidn2.0.dylib (libidn2)
  /usr/local/opt/libunistring/lib/libunistring.2.dylib (libunistring)
Indirect dependencies with linkage:
  gettext
  libunistring
Missing libraries:
  /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib (openssl)
  /usr/local/opt/openssl/lib/libssl.1.0.0.dylib (openssl)
@@ -21,7 +21,12 @@ def initialize(keg, formula = nil)
check_dylibs
end

def dylib_to_dep(dylib)
dylib.sub("#{HOMEBREW_PREFIX}/opt/", "").sub("#{HOMEBREW_CELLAR}/", "").split("/")[0]

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

dylib =~ %r{#{Regexp.escape(HOMEBREW_PREFIX)}/(opt|Cellar)/([\w+-.@]+)/} and then you can use the second capture group.

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 20, 2018

Contributor

display_items fails if dylib_to_dep returns nil, so I use "not a Homebrew library" as the default output. Here is how I tested it:

$ grep -A8 "def dylib_to_dep" ./Library/Homebrew/linkage_checker.rb 
  def dylib_to_dep(dylib)
    if dylib == "/usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib"
      nil
    elsif dylib =~ %r{#{Regexp.escape(HOMEBREW_PREFIX)}/(opt|Cellar)/([\w+-.@]+)/}
      Regexp.last_match(2)
    else
      "Not a Homebrew library"
    end
  end

$ brew install wget
$ brew uninstall --ignore-dependencies openssl
$ brew linkage wget
Error: comparison of Array with Array failed
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:175:in `sort'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:175:in `display_items'
/usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:120:in `display_normal_output'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/linkage.rb:28:in `block in linkage'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/linkage.rb:19:in `each'
/usr/local/Homebrew/Library/Homebrew/dev-cmd/linkage.rb:19:in `linkage'
/usr/local/Homebrew/Library/Homebrew/brew.rb:100:in `<main>'

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

@maxim-belkin I think it would be good to refactor that and handle the nil case instead.

missing = Set.new
@broken_dylibs.each_value do |value|
value.each do |str|
missing << dylib_to_dep(str) if str.start_with?("#{HOMEBREW_PREFIX}/opt", HOMEBREW_CELLAR)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 20, 2018

Member

You could check if this matches the dylib_to_dep regex above instead of having another start_with

maxim-belkin added some commits Mar 20, 2018

@@ -39,7 +46,7 @@ def check_dylibs
@system_dylibs << dylib
rescue Errno::ENOENT
next if harmless_broken_link?(dylib)
@broken_dylibs << dylib
@broken_dylibs[dylib_to_dep(dylib)] << dylib

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

What if dylib_to_dep(dylib) is nil?

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 21, 2018

Contributor

it just works :) (nil can be used as a key in hashes)

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

It feels very strange to do so?

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 21, 2018

Contributor

the result of dylib_to_dep is supposed to be the name of a Homebrew formula that provides that library. It seems reasonable for dylib_to_dep to return nil if missing library is not provided by Homebrew.

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 21, 2018

Contributor

is there a .keg_name method that we could apply to dylib?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

Use a separate list in that case, I'd say.

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 21, 2018

Contributor

Use a separate list

Can you clarify?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

Don't use @broken_dylibs with a nil key; use a separate instance variable for @broken_deps[dylib_to_dep(dylib)] and @broken_dylibs

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 30, 2018

Contributor

OK, makes sense. So when dylib_to_dep(dylib) resolves to a non-empty string - populate broken_dep, otherwise - broken_dylib

@@ -83,6 +91,7 @@ def check_undeclared_deps
next true if Formula[name].bin.directory?
@brewed_dylibs.keys.map { |x| x.split("/").last }.include?(name)
end
unnecessary_deps -= @broken_dylibs.map { |_, v| v.map { |d| dylib_to_dep(d) } }.flatten

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

Split this into a few lines:

  • Use a do .. end form each map
  • Set a new variable which you do the -= with

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

Also: you may be able to do |, v|

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 21, 2018

Contributor

At least locally, I need that _ in this implementation:

Error: /usr/local/Homebrew/Library/Homebrew/linkage_checker.rb:96: syntax error, unexpected ',', expecting '|'
    missing << @broken_dylibs.map do |, v|

I can use .values instead:

    missing_deps = @broken_dylibs.values.map do |v|
      v.map do |d|
        dylib_to_dep(d)
      end
    end.flatten.compact
    unnecessary_deps -= missing_deps
@@ -158,7 +167,7 @@ def display_items(label, things)
return if things.empty?
puts "#{label}:"
if things.is_a? Hash
things.sort.each do |list_label, list|
things.sort_by { |k, _| k.to_s }.each do |list_label, list|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Mar 21, 2018

Member

Think you can just do |k, |

This comment has been minimized.

@maxim-belkin

maxim-belkin Mar 21, 2018

Contributor

this works 😕

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Mar 21, 2018

Here is an example of brew linkage wget after forcefully uninstalling openssl:

$ brew linkage wget
System libraries:
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  /usr/lib/libSystem.B.dylib
  /usr/lib/libiconv.2.dylib
  /usr/lib/libz.1.dylib
Homebrew libraries:
  /usr/local/opt/gettext/lib/libintl.8.dylib (gettext)
  /usr/local/opt/libidn2/lib/libidn2.0.dylib (libidn2)
  /usr/local/opt/libunistring/lib/libunistring.2.dylib (libunistring)
Indirect dependencies with linkage:
  gettext
  libunistring
Missing libraries:
  /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib (openssl)
  /usr/local/opt/openssl/lib/libssl.1.0.0.dylib (openssl)

So, I question whether Indirect dependencies with linkage should be output here or in brew deps. Or, if here is fine, should they be printed at the very end?

@maxim-belkin maxim-belkin changed the title Fixes for linkage_checker linkage_checker: fix detection of broken dependencies and missing libraries Mar 30, 2018

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Mar 31, 2018

How to test:

# after force-removing openssl
$ brew linkage wget
System libraries:
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  /usr/lib/libSystem.B.dylib
  /usr/lib/libiconv.2.dylib
  /usr/lib/libz.1.dylib
Homebrew libraries:
  /usr/local/opt/gettext/lib/libintl.8.dylib (gettext)
  /usr/local/opt/libidn2/lib/libidn2.0.dylib (libidn2)
  /usr/local/opt/libunistring/lib/libunistring.2.dylib (libunistring)
Indirect dependencies with linkage:
  gettext
  libunistring
Missing dependencies:
  /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib (openssl)
  /usr/local/opt/openssl/lib/libssl.1.0.0.dylib (openssl)
@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Mar 31, 2018

In the current implementation of this PR:

when brew detects that a library is missing, it tries to identify corresponding dependency (dylib_to_dep). When this dependency is found, brew reports it under Missing dependencies, e.g.:

Missing dependencies:
  /usr/local/opt/<formula>/lib/libformula.1.0.0.dylib (<formula>)

if corresponding formula is not identified, it is reported in Missing libraries, e.g.:

Missing libraries:
  /usr/lib/libmissing.2.0.0.dylib

Alternative implementation could report all missing libraries and then missing dependencies:

Missing libraries:
  /usr/local/opt/<formula>/lib/libformula.1.0.0.dylib
  /usr/lib/libmissing.2.0.0.dylib
Missing dependencies:
  <formula>
@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Mar 31, 2018

audit was failing so had to use .sort_by(&:to_s)

@@ -83,6 +97,12 @@ def check_undeclared_deps
next true if Formula[name].bin.directory?
@brewed_dylibs.keys.map { |x| x.split("/").last }.include?(name)
end
missing_deps = @broken_deps.values.map do |v|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 7, 2018

Member

Could you use flat_map here to avoid the flatten and also possible the nested maps?

@@ -39,7 +47,12 @@ def check_dylibs
@system_dylibs << dylib
rescue Errno::ENOENT
next if harmless_broken_link?(dylib)
@broken_dylibs << dylib
dep = dylib_to_dep(dylib)
if dep.nil?

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 7, 2018

Member

if (dep = dylib_to_dep(dylib))

@@ -104,6 +124,7 @@ def display_normal_output
display_items "Indirect dependencies with linkage", @indirect_deps
display_items "Variable-referenced libraries", @variable_dylibs
display_items "Missing libraries", @broken_dylibs
display_items "Missing dependencies", @broken_deps

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 7, 2018

Member

What about missing installed dependencies or broken dependencies or similar; it's not clear from this whether they are missing because they aren't installed or missing because they aren't specified in the formulae?

This comment has been minimized.

@maxim-belkin

maxim-belkin Apr 7, 2018

Contributor

I likebroken dependencies better.

because they aren't specified in the formulae

This is undeclared_deps.

@@ -158,7 +180,7 @@ def display_items(label, things)
return if things.empty?
puts "#{label}:"
if things.is_a? Hash
things.sort.each do |list_label, list|
things.sort_by(&:to_s).each do |list_label, list|

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Apr 7, 2018

Member

This seems odd; do you want to sort by the key or value?

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 7, 2018

71fef14 needs to be fixed...

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 7, 2018

still broken. need to fix flat_map with Set

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 7, 2018

fixed, but this is all very fragile:

  • broken_deps has to use Array, not Set
  • flat_map does not replace flatten.map...
@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 7, 2018

We can also change the output to this:

$ brew linkage wget
System libraries:
  /System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation
  /usr/lib/libSystem.B.dylib
  /usr/lib/libiconv.2.dylib
  /usr/lib/libz.1.dylib
Homebrew libraries:
  /usr/local/opt/gettext/lib/libintl.8.dylib (gettext)
  /usr/local/opt/libidn2/lib/libidn2.0.dylib (libidn2)
  /usr/local/opt/libunistring/lib/libunistring.2.dylib (libunistring)
Indirect dependencies with linkage:
  gettext
  libunistring
Broken dependencies:
  openssl:
    /usr/local/opt/openssl/lib/libcrypto.1.0.0.dylib
    /usr/local/opt/openssl/lib/libssl.1.0.0.dylib

with the following patch:

$ git --no-pager diff
diff --git a/Library/Homebrew/linkage_checker.rb b/Library/Homebrew/linkage_checker.rb
index 75b50966c..a3862a83b 100644
--- a/Library/Homebrew/linkage_checker.rb
+++ b/Library/Homebrew/linkage_checker.rb
@@ -119,7 +119,7 @@ class LinkageChecker
     display_items "Indirect dependencies with linkage", @indirect_deps
     display_items "Variable-referenced libraries", @variable_dylibs
     display_items "Missing libraries", @broken_dylibs
-    display_items "Broken dependencies", @broken_deps
+    display_items "Broken dependencies", @broken_deps, true
     display_items "Undeclared dependencies with linkage", @undeclared_deps
     display_items "Dependencies with no linkage", @unnecessary_deps
   end
@@ -139,7 +139,7 @@ class LinkageChecker
 
   def display_test_output
     display_items "Missing libraries", @broken_dylibs
-    display_items "Broken dependencies", @broken_deps
+    display_items "Broken dependencies", @broken_deps, true
     display_items "Dependencies with no linkage", @unnecessary_deps
     puts "No broken dylib links" if @broken_dylibs.empty?
   end
@@ -171,13 +171,20 @@ class LinkageChecker
 
   # Display a list of things.
   # Things may either be an array, or a hash of (label -> array)
-  def display_items(label, things)
+  def display_items(label, things, group_by_label = false)
     return if things.empty?
     puts "#{label}:"
     if things.is_a? Hash
       things.keys.sort.each do |list_label|
-        things[list_label].sort.each do |item|
-          puts "  #{item} (#{list_label})"
+        if group_by_label
+          puts "  #{list_label}:"
+          things[list_label].sort.each do |item|
+            puts "    #{item}"
+          end
+        else
+          things[list_label].sort.each do |item|
+            puts "  #{item} (#{list_label})"
+          end
         end
       end
     else
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 8, 2018

I'm happy with the current output for now and did some basic testing of this locally. We can continue to iterate as desired. Thanks again @maxim-belkin!

@MikeMcQuaid MikeMcQuaid merged commit de82d3a into Homebrew:master Apr 8, 2018

2 of 3 checks passed

codecov/patch 44.44% of diff hit (target 70.05%)
Details
codecov/project 70.13% (+0.08%) compared to e8a4aad
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@maxim-belkin maxim-belkin deleted the maxim-belkin:fix-undeclared branch Apr 9, 2018

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 24, 2018

This actually broke brew linkage --test such that it doesn't detect broken dylibs anymore, which means anything without a test block has been sailing through CI even if it needed a revision bump and didn't get one. See Homebrew/homebrew-core#27008

The pianobar linkage to ffmpeg is broken and CI passed for pianobar in Homebrew/homebrew-core#26882

iMac-TMP:~ joe$ brew linkage --test pianobar
Broken dependencies:
  /usr/local/opt/ffmpeg/lib/libavcodec.57.dylib (ffmpeg)
  /usr/local/opt/ffmpeg/lib/libavfilter.6.dylib (ffmpeg)
  /usr/local/opt/ffmpeg/lib/libavformat.57.dylib (ffmpeg)
  /usr/local/opt/ffmpeg/lib/libavutil.55.dylib (ffmpeg)
Dependencies with no linkage:
  mad
No broken dylib links
iMac-TMP:~ joe$ echo $?
0
iMac-TMP:~ joe$ 
@GauthamGoli

This comment has been minimized.

Copy link
Member

GauthamGoli commented Apr 24, 2018

@ilovezfs

This comment has been minimized.

Copy link
Contributor

ilovezfs commented Apr 24, 2018

No. Checking for a test do block is a --strict audit only since there are still many formulae without one. We rely on brew linkage --test to protect the ones without a test do block, so we will now have to review everything from the last two weeks.

MikeMcQuaid added a commit to MikeMcQuaid/brew that referenced this pull request Apr 24, 2018

linkage: fix --test exit code.
Ensure that a non-zero exit code is set both for missing random dylibs
and random missing dependencies.

Additionally, while we are here, drastically trim down the public
interface for this class to the bare minimum and allow getting the
output from `display_test_output` as a variable.

Fixes issue mentioned by @ilovezfs in:
Homebrew#3940 (comment)
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 24, 2018

@ilovezfs This should fix the issue: #4106

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 24, 2018

@ilovezfs please write a new test for this while your memory is fresh. Thanks.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 24, 2018

@maxim-belkin No, this isn't the sort of thing that would or should be caught by an existing test, really. If you check out #4106 you'll see what changes should have been caught in review of this PR.

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 24, 2018

If this is a critical behavior (and according to @ilovezfs's comment it is) - not only it should be tested, it must be tested.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 24, 2018

@maxim-belkin If you feel so: you should write the test. I think it's a bit rich to be demanding that @ilovezfs write a test for the change that your PR broke and my PR just fixed. Even if a test was written for this already there's a strong chance it wouldn't have been caught. Additionally, writing that test now is unlikely to catch a future regression unless we made the same separation of formulae/non-formulae broken links again.

@maxim-belkin

This comment has been minimized.

Copy link
Contributor

maxim-belkin commented Apr 24, 2018

The fact that my PR broke the behavior that Homebrew relies upon highlights the need for such a test. So, I hope I'm not the only one who thinks that Homebrew/brew needs such test. All critical components/behaviors (return values, statuses, etc) have to have corresponding test(s). It might be even beneficial to create an issue to track down missing tests for all standard and developers' commands.

I'd be happy to write a test and will look into it. I invited @ilovezfs to do that because he was the first to spot it and knows exactly how to reproduce it.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Apr 24, 2018

The fact that my PR broke the behavior that Homebrew relies upon highlights the need for such a test.

A test would have been good and one that caught this would be good. The question is: would adding this test now catch any problems in future? Maybe, maybe not; I don't think it's certain for sure.

All critical components/behaviors (return values, statuses, etc) have to have corresponding test(s).

Yes, ideally. That said, there's a balance between writing said tests and other work.

I'd be happy to write a test and will look into it.

That would be good, thanks.

@lock lock bot added the outdated label May 24, 2018

@lock lock bot locked as resolved and limited conversation to collaborators May 24, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.